Skip to content

Conversation

@mxochicale
Copy link
Contributor

@mxochicale mxochicale commented Oct 15, 2024

Description

Fixes #511
Adds config files for mr-spectroscopy, including tags RF Echo Train Length Attribute and MR Timing and Related Parameters Sequence Attribute that James Moggridge mentioned is fine to keep them.

Type of change

Please delete options accordingly to the description.

  • New feature (non-breaking change which adds functionality)

Suggested Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have passed on my local host device. (see further details at the CONTRIBUTING document)
  • Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
  • I have requested PR review to UCLH-Foundtry/arc-dev
  • I have addressed and marked as resolved all the review comments in my PR.
  • Finally, I have selected squash and merge

@codecov
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.35%. Comparing base (83cc35a) to head (e8835ab).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #513   +/-   ##
=======================================
  Coverage   87.35%   87.35%           
=======================================
  Files          76       76           
  Lines        3378     3378           
=======================================
  Hits         2951     2951           
  Misses        427      427           

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

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.

Just want to check that you've gone through all the tags that are in both spectroscopy example studies? Surprising that there's only two extra tags on top of MRI at least at first glance

mxochicale and others added 4 commits October 16, 2024 18:23
…/mr-spectroscopy.yaml

Co-authored-by: Stef Piatek <s.piatek@ucl.ac.uk>
…_MR/1.dcm` which might be or not included (also needs to be formated). Do we need to include all tags with a config format?
@mxochicale
Copy link
Contributor Author

Just added dff1b18 with some ~20% of unformatted tags from dicom.dcmread(DICOM/csi_slaser_40_20_MR/1.dcm Do we need to include all tags of 1.dcm in the config format mr-spectroscopy.yaml? Maybe yes?, so you can then do more pre-processing and selection to where those tags belong to?

@mxochicale mxochicale requested a review from stefpiatek October 22, 2024 13:38
Copy link
Contributor

@p-j-smith p-j-smith left a comment

Choose a reason for hiding this comment

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

I've updated the mr-spectroscopy config file with tags that cause the validation to fail, and I've not yet included all nested tags except for those that are required by the validator

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.

let me know when you want a review but adding in a quick comment

@p-j-smith p-j-smith requested a review from stefpiatek October 28, 2024 12:58
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 getting this in, if you wanted to test this you create a temporary config using pixl__default__tag.yaml or whatever the placeholder value is when we add the pixl project tag to the instance on reciept. THen could upload some spectroscopy files manually into orthanc raw and that'll move it along to orthanc anon for anonymisation

@p-j-smith
Copy link
Contributor

if you wanted to test this you create a temporary config using pixl__default__tag.yaml or whatever the placeholder value is when we add the pixl project tag to the instance on reciept. THen could upload some spectroscopy files manually into orthanc raw and that'll move it along to orthanc anon for anonymisation

ah that's a good idea, thanks!

@p-j-smith p-j-smith requested a review from stefpiatek December 9, 2024 11:00
…ri.yaml

Also replace the tags rather than keep
@p-j-smith p-j-smith merged commit 3678f3c into main Dec 11, 2024
11 checks passed
@p-j-smith p-j-smith deleted the 511-anonymisation-spectroscopy-data branch December 11, 2024 10:39
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.

Create default anonymisation for spectroscopy data

4 participants