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

Change internal space/origin when using sft.to_x() with an empty sft. #2864

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

EmmaRenauld
Copy link
Contributor

@EmmaRenauld EmmaRenauld commented Aug 16, 2023

When using an empty SFT, internal space (sft._space) is never modified. Changing this ensures that we can compare the compatibility of many SFT, even when some are empty.

Same with origin.

@arokem
Copy link
Contributor

arokem commented Aug 16, 2023

This makes sense to me! Any chance to get a test of this functionality?

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #2864 (c54afdc) into master (a05e39d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2864   +/-   ##
=======================================
  Coverage   81.48%   81.48%           
=======================================
  Files         144      144           
  Lines       20055    20054    -1     
  Branches     3192     3192           
=======================================
  Hits        16341    16341           
  Misses       2906     2906           
+ Partials      808      807    -1     
Files Changed Coverage Δ
dipy/io/stateful_tractogram.py 75.37% <100.00%> (+0.18%) ⬆️

@frheault
Copy link
Contributor

Good catch ! This was indeed an oversight !

@EmmaRenauld
Copy link
Contributor Author

What type of test would you like? Here is an example of previous problematic usage:

sft1 = load_tractogram(some_path)
sft2 = sft1.from_sft([], sft)

print(sft1.space) --> RASMM
print(sft2.space) --> RASMM

sft1.to_vox()
sft2.to_vox()

print(sft1.space) --> VOX
print(sft2.space) --> RASMM

print(StatefulTractogram.are_compatible(sft1, sft2))  --> FALSE

I could add these lines somewhere, they should now be compatible. Where should I add it?

Can you help me understand why the Github tests were not successful?

@skoudoro
Copy link
Member

Can you help me understand why the Github tests were not successful?

You can ignore, this is not due to your PR. it seems there is an incompatibility between cvxpy and scipy on conda. We might need to pin specific version to avoid this issue

@skoudoro
Copy link
Member

skoudoro commented Aug 17, 2023

I could add these lines somewhere, they should now be compatible. Where should I add it?

You can add a new test in https://github.com/dipy/dipy/blob/master/dipy/io/tests/test_stateful_tractogram.py

Thanks !

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update @EmmaRenauld.

Looks good to me. I will wait until Monday to merge it in case there are any additional comments.

@skoudoro skoudoro merged commit 73b6704 into dipy:master Aug 21, 2023
25 of 27 checks passed
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.

None yet

4 participants