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

Add CheckDataMixin #568

Draft
wants to merge 20 commits into
base: gh/fzimmermann89/33/head
Choose a base branch
from

Conversation

fzimmermann89
Copy link
Member

@fzimmermann89 fzimmermann89 commented Nov 29, 2024

This adds a new Mixin for Dataclasses that checks the types of the fields.
If the fields are Annotated with a (newly added) Annotation, specifying either its data type or shape, this will also be checked.

After some discussions following #495, this is now optional. None of the core reshaping or indexing functions will depend on these checks, but the will be performed afterwards. If this ever becomes an issue, removing the mixin from the dataclass will only remove the checks.
This is IMHO the best way to check that certain invariants are true after reshaping or indexing in a readable and maintable way.
An alternative I considered was using data class descriptors instead of Annotated and perform the check in the set of the descriptors. But this would make the checking code fundamental to the data classes, instead of just a checking addon that can be removed.

One can specify the expected shape of a field using the syntax of jaxtyping.

There is also a context manager to disable checks temporarily.
By default, when exiting the context, the suspended checks will be performed. So one can first create a wrongly shaped data class, reshape it, and on exit it is ensured that the fields follow the rules. This behavior can also be disabled to disable checks in a part of the code completely.

The string parsing etc happens on import time, not at creation time.
Parts of the shape checks are cached, so running the same invariants check multiple times without substantial changes to the shapes will be fast.

I also removed some __future__.annotation imports, as they stringify all type annotations. Instead, we should either fix the dependencies, use stringified annotations (these cannot be runtime checked) or wait for PEP 649/749

[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Nov 29, 2024
ghstack-source-id: f63c6910796c61db777cdf0246e1d5edaec4e352
ghstack-comment-id: 2506886010
Pull Request resolved: #568
Copy link
Contributor

github-actions bot commented Nov 29, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   inati.py24196%44
   walsh.py16194%34
src/mrpro/algorithms/dcf
   dcf_voronoi.py53492%15, 48–49, 76
src/mrpro/algorithms/optimizers
   adam.py20195%69
src/mrpro/algorithms/reconstruction
   DirectReconstruction.py281643%51–71, 85
   IterativeSENSEReconstruction.py13192%76
   Reconstruction.py502256%42, 54–56, 80–87, 104–113
   RegularizedIterativeSENSEReconstruction.py411759%96–100, 114–139
src/mrpro/data
   AcqInfo.py146597%51, 55, 59, 229, 267
   CheckDataMixin.py34121836%94, 100, 102, 119, 127, 129, 131, 151–166, 177, 181–184, 188, 192–197, 201, 210, 214, 240–261, 289–322, 342–362, 444–449, 453–459, 478–515, 540–550, 578–584, 589–595, 607–613, 619–628, 648–652, 656, 662, 666–670, 674–677, 683–688, 724–756
   CsmData.py28389%13, 80–82
   DcfData.py44882%16, 64, 76–81
   EncodingLimits.py73396%33, 123, 126
   IData.py67987%119, 125, 129, 159–167
   IHeader.py75791%75, 109, 127–131
   KData.py2222788%112–113, 128, 135, 147–158, 167, 175, 184–188, 240–241, 281, 298, 303–304, 326–327, 487, 489, 562, 626, 635
   KHeader.py1391490%23, 108–112, 139, 187, 194–195, 198, 205, 222–229
   KNoise.py311552%39–52, 56–61
   KTrajectory.py81791%108–113, 116–118
   MoveDataMixin.py1401887%15, 113, 129, 143–145, 207, 323–325, 338, 417, 437–438, 440, 455–456, 458
   QData.py39782%42, 65–73
   Rotation.py6743595%100, 198, 335, 433, 477, 495, 581, 583, 592, 626, 628, 691, 768, 773, 776, 791, 808, 813, 889, 1077, 1082, 1085, 1109, 1113, 1240, 1242, 1250–1251, 1315, 1397, 1690, 1846, 1881, 1885, 1996
   SpatialDimension.py2322091%34, 104, 148, 154, 274–276, 289–291, 325, 343, 356, 369, 382, 395, 404–405, 420, 429
   acq_filters.py12192%47
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py27196%85
   KTrajectoryIsmrmrd.py13285%41, 50
src/mrpro/operators
   CartesianSamplingOp.py89397%118, 157, 280
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py65297%228, 234
   FiniteDifferenceOp.py27293%40, 105
   FourierOp.py158398%263, 381, 386
   Functional.py71593%20–22, 117, 119
   GridSamplingOp.py136993%72–73, 82–83, 90–91, 94, 96, 98
   LinearOperator.py1681094%55, 91, 190, 220, 261, 270, 278, 287, 295, 320
   LinearOperatorMatrix.py1581690%82, 119, 152, 161, 166, 175–178, 191–194, 203, 215, 304, 331, 359
   MultiIdentityOp.py13285%43, 48
   Operator.py78297%25, 74
   ProximableFunctionalSeparableSum.py39392%50, 103, 110
   SliceProjectionOp.py173895%44, 61, 63, 69, 206, 227, 260, 300
   WaveletOp.py120596%152, 170, 205, 210, 233
   ZeroPadOp.py16194%30
src/mrpro/utils
   filters.py62297%44, 49
   reshape.py60198%191
   slice_profiles.py46687%20, 36, 113–116, 149
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   summarize_tensorvalues.py11282%23, 25
   typing.py181139%8–23
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL524656789% 

Tests Skipped Failures Errors Time
2265 0 💤 2 ❌ 4 🔥 54.351s ⏱️

Copy link
Contributor

github-actions bot commented Nov 29, 2024

📚 Documentation

📁 Download as zip
🔍 View online

[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Nov 29, 2024
ghstack-source-id: f5a1fcae72994dc5ded3fda918068364da7a2617
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Nov 29, 2024
ghstack-source-id: bf978f509e37434b2a6e5680aa5238a8f2fdc12f
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Dec 2, 2024
ghstack-source-id: d3ba8f70111d645e2e8e51108770c33ee6265861
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Dec 2, 2024
ghstack-source-id: 61cd0d71ca65c1df4665dfb1de46d4113f5620e4
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Dec 2, 2024
ghstack-source-id: 139c4abb3716fcedc462c2f593e1f073e6fb56d5
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Dec 2, 2024
ghstack-source-id: cfe2e7e770ef021524e16a1f1531ec451aab3c17
ghstack-comment-id: 2506886010
Pull Request resolved: #568
[ghstack-poisoned]
fzimmermann89 added a commit that referenced this pull request Dec 2, 2024
ghstack-source-id: 0a9bcb0d6d68dd93ef6016d994f7dca796777305
ghstack-comment-id: 2506886010
Pull Request resolved: #568
@fzimmermann89 fzimmermann89 mentioned this pull request Dec 16, 2024
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89 fzimmermann89 mentioned this pull request Dec 23, 2024
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89 fzimmermann89 mentioned this pull request Dec 29, 2024
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@fzimmermann89 fzimmermann89 requested a review from ckolbPTB January 3, 2025 21:32
class CheckedDataClass(CheckDataMixin):
"""A test dataclass."""

float_tensor: Annotated[
Copy link
Member Author

Choose a reason for hiding this comment

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

See here for an example of the Annotations

)


def test_suspend_check_success() -> None:
Copy link
Member Author

Choose a reason for hiding this comment

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

see here (and next text) for the suspend checks context manager

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
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.

1 participant