Remove filtered displaySets from parameter from study protocolEngine.findMatch() #3343
Conversation
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-platform-viewer canceled.
|
|
Title: Question regarding the absence of updated this.studies when there are other expanded studies, unlike displaySets. Description: Can you please explain why this.studies is not updated in a similar way as the displaySets list when there are other expanded studies? Is there a reason for this difference in behavior? Thank you for your attention to this question. |
platform/core/src/services/HangingProtocolService/HangingProtocolService.ts
Outdated
Show resolved
Hide resolved
| const studyDisplaySets = this.displaySets.filter( | ||
| it => it.StudyInstanceUID === study.StudyInstanceUID | ||
| ); | ||
|
|
There was a problem hiding this comment.
If you don't move this change, but leave it here, then you can pass both displaySets and studyDisplaySets into the findMatch. Then, custom attributes can be setup to choose either of them - and it is only custom attributes which access these.
There was a problem hiding this comment.
Thinking it over, probably leaving displaySets as is, but passing allDisplaySets into the findMatch function would be more consistent. The rule is testing this study right now, so it should have the display sets for this study, not all display sets, and if a particular rule wants to look at all display sets, then just having it as a another value would make that possible.
There was a problem hiding this comment.
The problem is, at the study matching we need to access the list of registered study and display sets.
For unknown reason, this.studies is not updated when you add available studies in OHIF (for instance by opening priors panel), but displayset are correctly added.
At the study matching you need to access all studies and all display set (listing only displayset of the current study will forbid cross study matching for which study matching rule stand for).
So sofien updated his code to have :
displayset === all known displayset at the study level
displayset === studies display set at the series level
Codecov Report
@@ Coverage Diff @@
## master #3343 +/- ##
==========================================
+ Coverage 38.27% 38.36% +0.08%
==========================================
Files 82 82
Lines 1348 1345 -3
Branches 304 303 -1
==========================================
Hits 516 516
+ Misses 665 663 -2
+ Partials 167 166 -1 see 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Can you add a hanging protcol example and test set that requires this change to the basic test mode?
Also, I'd like to see this as an addition to the findMatch rules as opposed to a change to what is currently passed in. There are use cases this is needed, but having the display sets for the study is used in several existing hanging protocols, which would break them when being used with multi-study display sets.
|
The change only exposes the displayset at the study matching, at series level display set are only studies display sets. Did the study matching worked some days ? What we understood is that study matching as it was not passing displays set belonging to other studies was probably unable to match any other study outside the current one. (and neither pass other known studies as this.studies in HPservice is now updated to recieve new known studies) |
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Wait till the PR checks pass, then I can merge. |
|
Dear @wayfarer3130 should be good this one too ? |
Context
This pull request removes the studyDisplaySets parameter from the protocolEngine.findMatch() call in order to access a list of all expanded displaySets in studyMatchDetails.
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment