-
Notifications
You must be signed in to change notification settings - Fork 62
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
Hyperplane update #1034
base: master
Are you sure you want to change the base?
Hyperplane update #1034
Conversation
Hello @frheault, Thank you for updating !
Comment last updated at 2024-12-12 18:45:11 UTC |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1034 +/- ##
===========================================
- Coverage 69.96% 56.73% -13.24%
===========================================
Files 448 646 +198
Lines 24291 30209 +5918
Branches 3334 3413 +79
===========================================
+ Hits 16996 17139 +143
- Misses 5874 11633 +5759
- Partials 1421 1437 +16
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complicated code I don't fully understand, but apart from a few missing or incomplete docstring I didn't come across anything shocking. Also looks like there are a lot of remaining PEP8 issues.
@@ -117,8 +118,7 @@ def uniformize_bundle_sft(sft, axis=None, ref_bundle=None, swap=False): | |||
else: | |||
# Bitwise XOR | |||
if (bool(labels[tuple(sft.streamlines[i][0].astype(int))] > | |||
labels[tuple(sft.streamlines[i][-1].astype(int))]) | |||
^ bool(swap)): | |||
labels[tuple(sft.streamlines[i][-1].astype(int))])): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust you with the bitwise operations, but just want to make sure you didn't remove the ^ bool(swap)
part by accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok since the if at line 98 is swapping labels, this is checking orientation again based on that swap (so we don't want to cancel the previous swap)
@AntoineTheb Last time you tried that branch, everything was fine with the input/output? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments but LGTM:
flake8 scilpy/tractograms/streamline_and_mask_operations.py
scilpy/tractograms/streamline_and_mask_operations.py:634:5: F841 local variable 'orig_segment_len' is assigned to but never used
sft_centroid.to_vox() | ||
sft.to_corner() | ||
sft_centroid.to_corner() | ||
# TODO DOCSTRING ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frheault still missing docstring here
from scilpy.viz.color import get_lookup_table | ||
from scilpy.version import version_string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
from scilpy.version import version_string
Quick description
Attempt at having our improved RADTRACT Hyperplane subdivision !
https://www.ncbi.nlm.nih.gov/pmc/articles/PMC10246281/
...
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist