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

[WIP] feat: Save DICOM SR to the same series #3273

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

Conversation

wayfarer3130
Copy link
Contributor

Context

It becomes very confusing to have a lot of series all with the same series description, but differing date/times and content.
This PR changes the save so items get saved to the same series.

Changes & Results

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

@netlify
Copy link

netlify bot commented Mar 22, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 44dc3ba
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64c8103c12bcf70008507da0
😎 Deploy Preview https://deploy-preview-3273--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Mar 22, 2023

Deploy Preview for ohif-platform-viewer failed.

Name Link
🔨 Latest commit a6b93e5
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-viewer/deploys/64959c332747ab0008bad87d

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #3273 (b995a78) into master (72207e6) will decrease coverage by 5.59%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head b995a78 differs from pull request most recent head 44dc3ba. Consider uploading reports for the commit 44dc3ba to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3273      +/-   ##
==========================================
- Coverage   42.93%   37.34%   -5.59%     
==========================================
  Files          80       85       +5     
  Lines        1444     1403      -41     
  Branches      338      310      -28     
==========================================
- Hits          620      524      -96     
- Misses        661      707      +46     
- Partials      163      172       +9     
Files Changed Coverage Δ
platform/core/src/classes/MetadataProvider.js 4.16% <0.00%> (+0.08%) ⬆️

... and 31 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58d3849...44dc3ba. Read the comment docs.

@cypress
Copy link

cypress bot commented Mar 28, 2023

Passing run #3084 ↗︎

0 35 0 0 Flakiness 0

Details:

Fix merge issue
Project: Viewers Commit: b995a78289
Status: Passed Duration: 03:20 💡
Started: Apr 3, 2023 12:48 PM Ended: Apr 3, 2023 12:51 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@fedorov
Copy link
Member

fedorov commented Apr 3, 2023

@wayfarer3130 note that it is not the right DICOM practice to overwrite existing content. What if the prior version of the SR has already been used somewhere, is referenced from somewhere, or is indexed somewhere?

I understand sometime some deviation and compromises are unavoidable, but at the very least it should be possible to disable this feature, and it should be communicated to the user that an existing instance is being overwritten.

In general, I believe when using SR the idea is to use CompletionFlag attribute, in conjunction with references to prior evidence to track the chain of edits. I understand that the large number of reports can be overwhelming, but perhaps you can handle this in the UI by tracking the chain of edits and allowing to disable visualization of prior "versions".

If you have not yet read the section "Copies and Versions" of @dclunie "DICOM Structured Reporting" book, you are very strongly encouraged to do so!

@pieper
Copy link
Member

pieper commented Apr 4, 2023

@fedorov from a quick look I think @wayfarer3130 is not overwriting the SR instance, but adding the new instance to the same series as existing SRs. I don't know if this is allowed in the standard (not sure why it wouldn't be) but if there's a chain of edits of the "same" SR then to me it sounds good to put them in the same series.

@wayfarer3130
Copy link
Contributor Author

@fedorov from a quick look I think @wayfarer3130 is not overwriting the SR instance, but adding the new instance to the same series as existing SRs. I don't know if this is allowed in the standard (not sure why it wouldn't be) but if there's a chain of edits of the "same" SR then to me it sounds good to put them in the same series.

@fedorov - yes, I'm adding to the existing series. Ideally adding a deprecation reference to the earlier instances, but that isn't something I've implemented yet (I have to look up in the standard what the tags are for that - I've NEVER seen them used, but have seen them in the standard).

@dclunie
Copy link

dclunie commented Apr 4, 2023

"adding the new instance to the same series as existing SRs. I don't know if this is allowed in the standard (not sure why it wouldn't be)" - criteria for same Series are defined in the standard here - same equipment may be a barrier here, since arguably two deployed instances of the same viewer used on different computers at different times are not really "the same equipment".

@fedorov
Copy link
Member

fedorov commented Apr 7, 2023

@wayfarer3130 would you be interested to get together to discuss this feature together with the members of the Imaging Data Commons team (David Clunie, Steve Pieper and myself)? We would be interested in such a discussion.

@wayfarer3130 wayfarer3130 changed the base branch from v3-stable to master June 23, 2023 13:20
@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 44dc3ba
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/64c8103cf8a8670008b70a48
😎 Deploy Preview https://deploy-preview-3273--ohif-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sedghi
Copy link
Member

sedghi commented May 29, 2024

@wayfarer3130 Could you please revisit this after the new replies ? Thanks

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.

None yet

5 participants