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

Allow reference of optical path for annotations measurements #194

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

hackermd
Copy link
Collaborator

@hackermd hackermd commented Aug 5, 2022

In case of intensity measurements for annotations, the microscopy image (and the optical path) need to be specified for the measurements. This PR adds allows including a reference to the images to which the measurements apply.

cc @dclunie

@hackermd hackermd added the enhancement New feature or request label Aug 5, 2022
@hackermd hackermd requested a review from CPBridge August 5, 2022 18:49
@hackermd hackermd self-assigned this Aug 5, 2022
hackermd and others added 6 commits August 5, 2022 15:19
* Minor fixes to segmentation

* Fix docstring typo in type
* Fix recording of evidence in structured reports

* Assert that evidence is provided for references

* Use sets for comparison and avoid duplicates

* Add test for report referencing multiple studies

* Use datetime workaround for python 3.6 support

* Added image_library_entries to TID 1500

Co-authored-by: Sean Doyle <[email protected]>
Co-authored-by: hackermd <[email protected]>
Co-authored-by: Christopher Bridge <[email protected]>
Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

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

This looks good to me except one minor error I spotted. Though if I remember correctly, we were waiting on a decision about changes to the standard?

There are some changes in this PR that I think we should have in master now, regardless of what happens to the optical path stuff. Specifically the changes to the docstring of the ReferencedImageSequence (which replace the previous versions that are basically wrong) and the addition of the from_sequence method for that class.

Should we break this into 2 PRs and get these changes merged ASAP?

src/highdicom/ann/content.py Outdated Show resolved Hide resolved
List[CodedConcept],
np.ndarray,
List[CodedConcept],
List[Union[ReferencedImageSequence, None]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

NB backwards incompatible change

@hackermd
Copy link
Collaborator Author

This looks good to me except one minor error I spotted. Though if I remember correctly, we were waiting on a decision about changes to the standard?

We have submitted a Correction Proposal and are currently discussing it with the DICOM Standard Committee. cc @dclunie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants