[MRG] Correct waveform calculation. Closes #1667 #1715
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1715 +/- ##
==========================================
+ Coverage 97.59% 97.61% +0.01%
==========================================
Files 66 66
Lines 10769 10769
==========================================
+ Hits 10510 10512 +2
+ Misses 259 257 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thank you! Please also add an entry to the release notes and, if possible, a test verifying the fix.
| arr[..., jj] = ( | ||
| (arr[..., jj] + baseline) * sensitivity * correction | ||
| arr[..., jj] * sensitivity * correction + baseline | ||
| ) |
There was a problem hiding this comment.
Looks correct to me, but I would to have someone else have a look with a better understanding of waveforms (@scaramallion ?). I guess it is a bit diffcult to write a test here...
There was a problem hiding this comment.
I did try to verify the math (not something I've had experience with). The standard does note that baseline is in sensitivity units (verifying the comment in #1667: "It is already in "sensitivity" units"). It is also a VR of DS in the dictionary, which makes sense, vs an int matching the raw data type.
Note that the pydicom docs on this also include this exact equation in the text, so also need to be updated.
doc/tutorials/waveforms.rst
Outdated
| in the quantity it represents. This correction is given by (sample + *Channel | ||
| Baseline*) x *Channel Sensitivity* x *Channel Sensitivity Correction Factor* | ||
| in the quantity it represents. This correction is given by sample x *Channel | ||
| Sensitivity* x *Channel Sensitivity Correction Factor*+ *Channel Baseline* |
There was a problem hiding this comment.
Nitpicking: please add a space before the "+"
pydicom/waveforms/numpy_handler.py
Outdated
| correction = ch.get("ChannelSensitivityCorrectionFactor", 1.0) | ||
| arr[..., jj] = ( | ||
| (arr[..., jj] + baseline) * sensitivity * correction | ||
| arr[..., jj] * sensitivity * correction + baseline |
There was a problem hiding this comment.
Sorry to nitpick, but I still see a warning here in the diff view for an extra space we might as well correct.
@mrbean-bremen, can we make these show up inline in Conversation tab? They don't show up in CI unless you click through to details.
There was a problem hiding this comment.
I don't know, but I don't think so, as this comes from the GitHub API, not from the action itself.
We could of course convert these warnings to errors, and let the build fail on errors - that would show it more clearly, though I don't like it too much.
Another possibility is to use pre-commit instead of reviewdog - this can be installed locally via pip and will install a pre-commit hook, that will prevent a commit on any error. There is also a related CI that uses the pre-commit configuration and will fail the build in case of errors, and also create PRs for tool updates. I have been made aware of this recently and have used it for black and flake8 (this doesn't really belong here - we may discuss this elsewhere).
There was a problem hiding this comment.
this doesn't really belong here - we may discuss this elsewhere
Agreed! Just wanted to point it out in case it was helpful to see the outputs.
|
What is the next step and what can I do about it? |
|
Looks like I have introduced conflicts with my latest change - I can resolve them tonight, or you could do this (e.g. rebase against master and resolve the conflicts, should be trivial). After that, the mypy checks should also pass. |
pydicom/valuerep.py
Outdated
| # If a string passed, then store it | ||
| if isinstance(val, str): | ||
| self.original_string = val.strip() | ||
| elif isinstance(val, (IS, ISfloat)) and hasattr(val, 'original_string'): |
There was a problem hiding this comment.
Reviewdog complains about this line that is 1 character too long. As long as we haven't changed this policy, this needs to be fixed... I'm not sure why codecov also complains, but I would say it got confused and we can ignore this.
@darcymason - I think we can merge this as soon as this minor formatting issue is fixed.
There was a problem hiding this comment.
Reviewdog complains about this line that is 1 character too long. As long as we haven't changed this policy, this needs to be fixed
Looks like merge is not blocked, and this won't be a problem when we update the settings to allow longer lines, so I'm happy to leave it.
I'm not sure why codecov also complains, but I would say it got confused and we can ignore this
That seems to happen quite frequently, at some point we should try to understand these differences, but for now I am happy to ignore.
pydicom/valuerep.py
Outdated
| self.original_string = val.strip() | ||
| elif isinstance(val, (IS, ISfloat)) and hasattr(val, 'original_string'): | ||
| elif isinstance(val, (IS, ISfloat)) | ||
| and hasattr(val, 'original_string'): |
There was a problem hiding this comment.
You need brackets here, e.g.
elif (isinstance(val, (IS, ISfloat))
and hasattr(val, 'original_string')):
pydicom/valuerep.py
Outdated
| self.original_string = val.strip() | ||
| elif isinstance(val, (IS, ISfloat)) | ||
| and hasattr(val, 'original_string'): | ||
| elif (isinstance(val, (IS, ISfloat)) |
There was a problem hiding this comment.
Now it complains about a trailing space... quite pedantic. For the future, we really may want to introduce auto-correct for these kinds of problems...
Solves issue #1667