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

Angle and CobbAngle tool: set default 1 for rowPixelSpacing and columnPixelSpacing #1229

Merged
merged 8 commits into from
May 29, 2020

Conversation

aminica
Copy link
Contributor

@aminica aminica commented Apr 28, 2020

Only added in the Angle and CobbAngle tool: rowPixelSpacing and columnPixelSpacing default to 1

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Bug fix

  • What is the current behavior? (You can also link to an open issue here)
    When there are no pixel spacing data of the image, the values of the angle measurements are not able to be calculated, since the getPixelSpacing function returns null for the rowPixelSpacing and columnPixelSpacing values.

  • What is the new behavior (if this is a feature change)?
    Value 1 is used in the angle value calculation if the rowPixelSpacing and/or columnPixelSpacing values are null.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No.

  • Other information:
    Default value 1 is added since in the past every tool had it, but due to some refactoring (in which the getPixelSpacing function was created), this scenario was left out.

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #1229 into master will increase coverage by 0.32%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1229      +/-   ##
==========================================
+ Coverage   18.71%   19.03%   +0.32%     
==========================================
  Files         286      286              
  Lines        8583     8584       +1     
  Branches     1459     1459              
==========================================
+ Hits         1606     1634      +28     
+ Misses       5784     5759      -25     
+ Partials     1193     1191       -2     
Impacted Files Coverage Δ
src/tools/annotation/CobbAngleTool.js 0.00% <ø> (ø)
src/tools/annotation/AngleTool.js 27.36% <100.00%> (+27.36%) ⬆️
src/util/roundToDecimal.js 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

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

@dannyrb
Copy link
Member

dannyrb commented Apr 28, 2020

@JamesAPetts, this doesn't change labeling from px to mm, but it does assume square pixels. That feels like a safer assumption. Thoughts on merging this?

@dannyrb dannyrb requested review from dannyrb and JamesAPetts April 28, 2020 10:37
@sroopra
Copy link

sroopra commented May 13, 2020

Any idea when this will be merged? Or is it blocked by the codeclimate issue?

@aminica
Copy link
Contributor Author

aminica commented May 15, 2020

So, all checks have now passed. When could we expect the merge?

@milosdjurdjevic
Copy link

@dannyrb some info on this?

@aminica
Copy link
Contributor Author

aminica commented May 28, 2020

@JamesAPetts can we have some update on this?

@dannyrb
Copy link
Member

dannyrb commented May 29, 2020

Appreciate your patience on this one. I'll merge, but may revert if someone comes forward and forces the non-square pixel argument.

We may eventually want this behavior as an optional flag that can be sit when cornerstone.init() is called.

@dannyrb dannyrb merged commit d16ecd4 into cornerstonejs:master May 29, 2020
@dannyrb
Copy link
Member

dannyrb commented May 29, 2020

🎉 This PR is included in version 4.15.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aminica
Copy link
Contributor Author

aminica commented May 29, 2020

@dannyrb Thank you very much! :)

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

Successfully merging this pull request may close these issues.

4 participants