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.

Sign up for GitHub

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

Jump to bottom

Allow customisable tag operations per MR model #354

Merged
merged 47 commits into from
Mar 27, 2024
Merged

Allow customisable tag operations per MR model #354

merged 47 commits into from
Mar 27, 2024

Conversation

milanmlft
Copy link
Member

  • Add example tags for MRI diffusion data
  • Update test config to take manufacturer overrides
  • Update project config template with base and manufacturer overrides
  • Update project config handler to allow manufacturer overrides
  • Add simple test for merging multiple tag schemes
  • project_config: make manufacturer overrides optional
  • project_config: export TagOperations
  • Refactor merge_tag_schemes so it can handle manufacturer overrides
  • Update test for base-only tag scheme
  • Add test fixture for manufacturer overridden tag scheme
  • Test if manufacturer overrides work
  • Actually apply the manufacturer overrides during anonymisation

Fixes #292

@milanmlft
Add example tags for MRI diffusion data
413fd21 
Using 2 manufacturers for testing
@milanmlft
Update test config to take manufacturer overrides
b7d798b
@milanmlft
Update project config template with base and manufacturer overrides
6ef1fc6
@milanmlft
Update project config handler to allow manufacturer overrides
17a6c4f
@milanmlft
Add note on implementing merge_tag_schemes for multiple schemes
723abd0
@milanmlft
Add simple test for merging multiple tag schemes
f566a61
@milanmlft
Merge branch 'main' into milanmlft/292-allow-dicom-tag-operations-to-…
a92e52f 
…be-customised-per-mr-model
@milanmlft
Merge branch 'main' into milanmlft/292-allow-dicom-tag-operations-to-…
8b58656 
…be-customised-per-mr-model
@milanmlft
project_config: make manufacturer overrides optional
7e0208d
@milanmlft
project_config: export TagOperations
6c567c5 
So we can use it to access the tag operation files from other modules
@milanmlft
Refactor merge_tag_schemes so it can handle manufacturer overrides
1433461
@milanmlft
Update test for base-only tag scheme
1ee1e8e
@milanmlft
Add test fixture for manufacturer overridden tag scheme
1df3e5c
@milanmlft
Test if manufacturer overrides work
a91149a
@milanmlft
Actually apply the manufacturer overrides during anonymisation
06f045b
@milanmlft
Update docs
eeba06a
@milanmlft milanmlft marked this pull request as ready for review March 21, 2024 19:45
@milanmlft milanmlft requested a review from a team March 21, 2024 19:45
stefpiatek
stefpiatek approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Milan, looks good - some suggestions here and there, happy to chat if morning me hasn't made much sense

pixl_core/src/core/project_config.py Outdated Show resolved Hide resolved
pixl_core/tests/test_project_config.py Outdated Show resolved Hide resolved
pixl_core/tests/test_project_config.py Outdated Show resolved Hide resolved
pixl_core/tests/test_project_config.py Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/main.py Outdated Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/main.py Outdated Show resolved Hide resolved
pixl_dcmd/tests/test_main.py Outdated Show resolved Hide resolved
projects/configs/uclh-nasogastric-tube-project-ngt-only-full-dataset.yaml Outdated Show resolved Hide resolved
projects/configs/tag-operations/mri-diffusion.yaml Outdated Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/main.py Outdated Show resolved Hide resolved
@milanmlft milanmlft mentioned this pull request Mar 25, 2024
Merged
@milanmlft
Better naming for pydantic class holding paths to tagops files
4807690
@milanmlft
Refactor tagoperations and make it a public module
6f806d5
@milanmlft
Export tagoperations functionality in project_config.__init__.py
9f27fef
@milanmlft
Update project config tests
9b40a25
@milanmlft
Don't store project config in TagOperations
f318977
@milanmlft
Test whether loading tag operations works
a3d0917
@milanmlft
Refactor: move tag scheme merging to separate module
ead2c1e
@milanmlft
Add more validation checks for tags
19812b1
@milanmlft
Get rid of TagScheme
fdbd8b3
@milanmlft
Add more validation checks for valid tag formats
4a0d9a6
@milanmlft
Add tests for valid tag formats
3a984f1
@milanmlft
Fix pixl_dcmd._tagschemes: no more TagScheme
7005481 
See also fdbd8b3
@milanmlft
Move tagscheme tests to separate test file
ea2e2e3
@milanmlft
Fix pixl_dcmd main tests: correctly set tag scheme for tests
0a98681
@milanmlft
Merge branch 'main' into milanmlft/292-allow-dicom-tag-operations-to-…
d909e63 
…be-customised-per-mr-model
@milanmlft
Copy link
Member Author

Started adding unit test for anonymise_dicom() but will take a bit more work, so deferring to #360.
For now, system test should catch any problems with the complete anonymisation.

stefpiatek
stefpiatek approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for this Milan. Mostly some file naming suggestions

pixl_core/src/core/project_config/tagoperations.py Outdated Show resolved Hide resolved
pixl_core/src/core/project_config/pixlconfig_model.py Outdated Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/_tagschemes.py Outdated Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/main.py Outdated Show resolved Hide resolved
pixl_dcmd/src/pixl_dcmd/_tagschemes.py Outdated Show resolved Hide resolved
pixl_dcmd/tests/test_tagschemes.py Outdated Show resolved Hide resolved
milanmlft and others added 3 commits March 27, 2024 11:23
@milanmlft milanmlft enabled auto-merge (squash) March 27, 2024 13:55
@milanmlft milanmlft merged commit 85257cb into main Mar 27, 2024
8 checks passed
@milanmlft milanmlft deleted the milanmlft/292-allow-dicom-tag-operations-to-be-customised-per-mr-model branch March 27, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefpiatek stefpiatek stefpiatek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow DICOM tag operations to be customised per MR model
2 participants
@milanmlft @stefpiatek