Closed EmmaRenauld closed 3 months ago
Hello @EmmaRenauld, Thank you for submitting the Pull Request !
scripts/scil_frf_memsmt.py
:Line 237:80: E501 line too long (80 > 79 characters)
Attention: Patch coverage is 88.77551%
with 22 lines
in your changes are missing coverage. Please review.
Project coverage is 66.95%. Comparing base (
546189e
) to head (8125862
). Report is 36 commits behind head on master.
Really nice addition! I found a few scripts where I don't understand why we don't use your function
assert_headers_compatible
, especially when we have multiple tractograms (in scil_tractogram or scil_bundle). I also added a few esthetic comments. Moreover, I think some scripts might have been left out, likescil_dwi_concatenate.py
, where a check is done withis_header_compatible
but could be replaced by your function.Good job!
Yeah, at first I was managing only script calling the function get_data_as_mask. Then I also managed all scripts using add_reference_arg. But I didn't go through all scripts exhaustively. Should I? In a new PR?
Quick description
I was seeing many times the same line:
Replaced by single-line with verification inside get_data_as_mask. Makes the verification more thorough (was not done in all scripts) and will help with coverage.
EDIT:
After discussion with Arnaud, instead, not verifying shape there, but using assert_headers_compatible at the beginning.
assert_inputs_exit
calls.assert_inputs_exit
calls. ...Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist