Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(getPixelSpacingInformation): treat negative pixel spacing #3860

Conversation

pedrokohler
Copy link
Collaborator

@pedrokohler pedrokohler commented Dec 16, 2023

Context

Currently negative values of pixel spacing aren't being properly treated at OHIF and this inverts the image. The solution is to force them positive.

Changes & Results

Before:

image

After:

image

Testing

Manually tested.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: MacOS 14.0 (23A344)
  • Node version: 18.16.1
  • Browser: Chrome 119.0.6045.105

Copy link

netlify bot commented Dec 16, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 5851e8a
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/657da5a12efb4f0008bbbaa6
😎 Deploy Preview https://deploy-preview-3860--ohif-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 16, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 5851e8a
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/657da5a053b6050008d1f3c0
😎 Deploy Preview https://deploy-preview-3860--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Merging #3860 (5851e8a) into master (8a335bd) will decrease coverage by 1.80%.
Report is 181 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3860      +/-   ##
==========================================
- Coverage   46.23%   44.44%   -1.80%     
==========================================
  Files          78       80       +2     
  Lines        1276     1332      +56     
  Branches      312      327      +15     
==========================================
+ Hits          590      592       +2     
- Misses        548      587      +39     
- Partials      138      153      +15     
Files Coverage Δ
platform/app/src/routes/WorkList/filtersMeta.js 0.00% <ø> (ø)
platform/core/src/utils/addAccessors.js 9.09% <ø> (ø)
platform/core/src/utils/downloadCSVReport.js 0.00% <ø> (ø)
platform/core/src/utils/formatDate.js 0.00% <ø> (ø)
platform/core/src/utils/index.js 100.00% <ø> (ø)
...form/core/src/utils/isDisplaySetReconstructable.js 5.00% <ø> (+0.18%) ⬆️
...ils/metadataProvider/getPixelSpacingInformation.js 0.00% <ø> (ø)
platform/core/src/log.js 14.28% <0.00%> (-85.72%) ⬇️
...services/DicomMetadataStore/createStudyMetadata.js 0.00% <0.00%> (ø)
...ervices/DicomMetadataStore/createSeriesMetadata.js 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb7c950...5851e8a. Read the comment docs.

@sedghi
Copy link
Member

sedghi commented Dec 18, 2023

can you try removing the whole ultrasound enhanced region logic here, and upgrade cs3d since that implements it properly

@sedghi
Copy link
Member

sedghi commented May 21, 2024

@pedrokohler can you try latest master and let me know if you need this fix or not

@pedrokohler
Copy link
Collaborator Author

@pedrokohler can you try latest master and let me know if you need this fix or not

Yes, it looks like it's fixed.

Old behavior:
image

Current behavior:
image

I'm closing the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants