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

ENH: Refactor data representation structures for multimodality #52

Merged
merged 13 commits into from
Jan 20, 2025

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Jan 17, 2025

Proposes a new nifreeze.data.base.BaseDataset class with minimal metadata shared across modalities. Modality-wise data representation classes should derive from this one and extend the necessary behaviors (e.g., indexed access).

I believe this approach would be cleaner to have a common interface. Models can be more specific in the required data structures they handle.

By adding indexing, the dataset can be split with fancy indexing, but returning a tuple containing the split across data AND metadata. This will allow transparent splits within models.

Resolves: #19.

@oesteban oesteban marked this pull request as draft January 17, 2025 10:29
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 71.68142% with 64 lines in your changes missing coverage. Please review.

Project coverage is 67.60%. Comparing base (fdab87b) to head (68ccc85).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/nifreeze/data/pet.py 24.00% 38 Missing ⚠️
src/nifreeze/data/base.py 86.17% 8 Missing and 5 partials ⚠️
src/nifreeze/data/dmri.py 87.17% 4 Missing and 6 partials ⚠️
src/nifreeze/registration/ants.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   65.85%   67.60%   +1.75%     
==========================================
  Files          19       20       +1     
  Lines         943      991      +48     
  Branches      121      131      +10     
==========================================
+ Hits          621      670      +49     
+ Misses        277      267      -10     
- Partials       45       54       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oesteban oesteban marked this pull request as ready for review January 17, 2025 14:42
Copy link
Contributor

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Approving for the sake of moving forward: comments can be addressed in a separate PR.

So this means I will need to fix all conflicts in PR #28 😭.

Thanks for this, thanks for the notebook.

Copy link

@arokem arokem left a comment

Choose a reason for hiding this comment

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

Just a couple of thoughts about whether we want something more generic for the stored affines. But I have not thought this through very deeply.

@oesteban
Copy link
Member Author

So this means I will need to fix all conflicts in PR #28 😭.

See 68ccc85 to minimize those conflicts.

@oesteban oesteban merged commit 0dadde7 into main Jan 20, 2025
8 checks passed
@oesteban oesteban deleted the enh/data-refactor branch January 20, 2025 09:38
oesteban added a commit that referenced this pull request Jan 20, 2025
Leverage the new ``__getitem__`` interface, which provides uniform
indexed access across modalities.

Requires: #52.
Related-To: #19.
Resolves: #20.
@jhlegarreta
Copy link
Contributor

See 68ccc85 to minimize those conflicts.

Thanks.

oesteban added a commit that referenced this pull request Jan 20, 2025
Leverage the new ``__getitem__`` interface, which provides uniform
indexed access across modalities.

Requires: #52.
Related-To: #19.
Resolves: #20.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the data representation structure more generic to accommodate both DWI and PET
3 participants