fix(pt): resolve Philips PET private SUV bulkdata before scaling#6096
Open
sedghi wants to merge 1 commit into
Open
fix(pt): resolve Philips PET private SUV bulkdata before scaling#6096sedghi wants to merge 1 commit into
sedghi wants to merge 1 commit into
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a Philips PET private scalar bulkdata resolution pipeline: ChangesPET Bulkdata Scalar Resolution Pipeline
Sequence DiagramsequenceDiagram
participant DicomWebDataSource
participant addRetrieveBulkData
participant qidoDicomWebClient
participant resolvePETPrivateScalarBulkData
participant getPTImageIdInstanceMetadata
DicomWebDataSource->>addRetrieveBulkData: data.map(addRetrieveBulkData)
addRetrieveBulkData->>qidoDicomWebClient: bind retrieveBulkData to BulkDataURI entries
addRetrieveBulkData-->>DicomWebDataSource: naturalized instances
DicomWebDataSource->>resolvePETPrivateScalarBulkData: await resolvePETPrivateScalarBulkData(instances)
resolvePETPrivateScalarBulkData->>qidoDicomWebClient: retrieveBulkData() for tags 70531000/70531009
qidoDicomWebClient-->>resolvePETPrivateScalarBulkData: raw byte payload
resolvePETPrivateScalarBulkData->>resolvePETPrivateScalarBulkData: decodeNumericBulkData (text or binary float)
resolvePETPrivateScalarBulkData-->>DicomWebDataSource: instances mutated with numeric scalars
DicomWebDataSource->>getPTImageIdInstanceMetadata: build instance metadata
getPTImageIdInstanceMetadata->>getPTImageIdInstanceMetadata: coerceNumber on all numeric fields
getPTImageIdInstanceMetadata-->>DicomWebDataSource: instanceMetadata with resolved PET values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Viewers
 |
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
fix/pt-suv-philips-bulkdata
|
| Run status |
|
| Run duration | 01m 48s |
| Commit |
|
| Committer | Alireza |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
28
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@extensions/default/src/DicomWebDataSource/utils/resolvePETPrivateScalarBulkData.ts`:
- Around line 148-153: The resolvePETPrivateScalarBulkData function uses
unbounded parallelism with Promise.all(instances.map(resolveInstance)), which
can trigger excessive concurrent bulkdata requests and overwhelm the DICOMweb
endpoint. Implement a concurrency limit to control the fan-out by processing
instances in batches or using a concurrency control mechanism (such as a
queue-based approach or a concurrency limiter utility) that allows only a fixed
number of resolveInstance calls to execute simultaneously, ensuring the bulkdata
resolution respects reasonable throughput constraints.
In `@extensions/default/src/getPTImageIdInstanceMetadata.ts`:
- Around line 21-23: The numeric conversion in the string value handling is
converting empty or whitespace-only strings to zero through Number(value), which
silently treats missing DICOM fields as valid zeros. Before calling
Number(value), add a check to ensure the string is not empty or contains only
whitespace; if it is blank, return undefined directly instead of proceeding with
the Number conversion. Only call Number(value) and check Number.isFinite(n) if
the string has actual content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80a683b1-39d9-4fcd-a20c-60199ed5218e
📒 Files selected for processing (3)
extensions/default/src/DicomWebDataSource/index.tsextensions/default/src/DicomWebDataSource/utils/resolvePETPrivateScalarBulkData.tsextensions/default/src/getPTImageIdInstanceMetadata.ts
Comment on lines
+148
to
+153
| export async function resolvePETPrivateScalarBulkData(instances): Promise<void> { | ||
| if (!Array.isArray(instances) || !instances.length) { | ||
| return; | ||
| } | ||
| await Promise.all(instances.map(resolveInstance)); | ||
| } |
There was a problem hiding this comment.
Limit bulkdata resolution fan-out to avoid request storms.
Promise.all(instances.map(resolveInstance)) triggers unbounded parallel bulkdata fetches on large PT series. This can overload DICOMweb endpoints and destabilize ingestion latency.
Suggested fix
export async function resolvePETPrivateScalarBulkData(instances): Promise<void> {
if (!Array.isArray(instances) || !instances.length) {
return;
}
- await Promise.all(instances.map(resolveInstance));
+ for (const instance of instances) {
+ await resolveInstance(instance);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function resolvePETPrivateScalarBulkData(instances): Promise<void> { | |
| if (!Array.isArray(instances) || !instances.length) { | |
| return; | |
| } | |
| await Promise.all(instances.map(resolveInstance)); | |
| } | |
| export async function resolvePETPrivateScalarBulkData(instances): Promise<void> { | |
| if (!Array.isArray(instances) || !instances.length) { | |
| return; | |
| } | |
| for (const instance of instances) { | |
| await resolveInstance(instance); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@extensions/default/src/DicomWebDataSource/utils/resolvePETPrivateScalarBulkData.ts`
around lines 148 - 153, The resolvePETPrivateScalarBulkData function uses
unbounded parallelism with Promise.all(instances.map(resolveInstance)), which
can trigger excessive concurrent bulkdata requests and overwhelm the DICOMweb
endpoint. Implement a concurrency limit to control the fan-out by processing
instances in batches or using a concurrency control mechanism (such as a
queue-based approach or a concurrency limiter utility) that allows only a fixed
number of resolveInstance calls to execute simultaneously, ensuring the bulkdata
resolution respects reasonable throughput constraints.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
When a DICOMweb server delivers the Philips PET private tags SUVScaleFactor
(7053,1000) / ActivityConcentrationScaleFactor (7053,1009) as bulkdata, dcmjs
naturalization leaves them as { BulkDataURI } objects. These were fed verbatim
to calculate-suv, which treats the object as a valid value and silently
corrupts the SUV scaling factors.
Resolve these scalar private tags to numbers during ingestion - in both the
lazy (async) and non-lazy (sync) DICOMweb metadata paths, before INSTANCES_ADDED
fires - by decoding the bulkdata (VR-aware: DS/IS text or little-endian FL/FD).
Harden getPTImageIdInstanceMetadata to coerce values to finite numbers (reusing
@OHIF/core utils.toNumber) and reject unresolved bulkdata objects so they can
never reach calculate-suv. Share the bulkdata-attach helper between both
metadata paths. Adds unit tests for the bulkdata decoder/resolver and for
getPTImageIdInstanceMetadata.
sedghi
force-pushed
the
fix/pt-suv-philips-bulkdata
branch
from
ea7c131 to
249de93
Compare
sedghi
commented
Jun 21, 2026
|
|
||
| // Resolve PET private scalar tags (e.g. the Philips SUV Scale Factor) | ||
| // delivered as bulkdata into plain numbers BEFORE INSTANCES_ADDED fires. | ||
| await resolvePETPrivateScalarBulkData(naturalizedInstancesMetadata); |
Member
Author
There was a problem hiding this comment.
I really hate this solution, open to what you think is best, the issue is dicomimageloader needs this tags for scaling at earliest time before decodeimageframe
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a DICOMweb server delivers the Philips PET private tags SUVScaleFactor
(7053,1000)and ActivityConcentrationScaleFactor(7053,1009)as bulkdata, dcmjs naturalization leaves them as{ BulkDataURI: '...' }objects under their raw hex keys. OHIF's MetadataProvider returns them verbatim and nothing resolves them.getPTImageIdInstanceMetadatathen copied those objects straight intoPhilipsPETPrivateGroup.SUVScaleFactor, and@cornerstonejs/calculate-suvhas no bulkdata awareness — its validity guards (!== null/undefined/0) all pass for an object, so forUnits === 'CNTS'it returns the{ BulkDataURI }object verbatim assuvbw(or multiplies toNaN). No exception is thrown, so SUV scaling is silently corrupted.Fix
INSTANCES_ADDEDfires, in both the lazy (_retrieveSeriesMetadataAsync→storeInstances) and non-lazy (_retrieveSeriesMetadataSync) DICOMweb metadata paths. Resolution reuses the existingvalue.retrieveBulkDataresolver and decodes the buffer VR-aware (printable ASCII →DS/IStext; otherwise little-endianFL/FD). New helper:resolvePETPrivateScalarBulkData.ts.addRetrieveBulkData) is hoisted to the data-source scope so both metadata paths share it (previously only the lazy path attachedretrieveBulkData).getPTImageIdInstanceMetadata: coerce values to finite numbers and reject objects (a{ BulkDataURI }can never reach calculate-suv), fix the broken Philips-tag guard, readRadiopharmaceuticalInformationSequencerobustly (array or object), and import types from the package root instead of@cornerstonejs/calculate-suv/src/types.Notes
This addresses the metadata/SUV-factor side. Rendering prescaled SUV for PET stored with an identity modality LUT (Philips
CNTS,rescaleSlope === 1) additionally requires cornerstonejs/cornerstone3D#2767.Verification
Tested live against a Philips whole-body PET (
Units: CNTS, 255 instances) served via DICOMweb: the private tags resolve to numbers (e.g. SUVScaleFactor0.00038, decoded from theDSbulkdata"0.00038 "),scalingModule.suvbwis populated for all 255 instances, and SUV scaling renders correctly.Summary by CodeRabbit
Release Notes
Greptile Summary
This PR fixes silent SUV corruption on Philips PET studies served by DICOMweb servers that deliver the private scalar tags
(7053,1000)and(7053,1009)as bulkdata. The fix has two complementary layers: an upstream resolver (resolvePETPrivateScalarBulkData) that decodes the bulkdata into a plain number beforeINSTANCES_ADDEDfires, and a downstream backstop (coerceNumber) insidegetPTImageIdInstanceMetadatathat prevents any unresolved{ BulkDataURI }object from ever reachingcalculate-suv.resolvePETPrivateScalarBulkDatahelper auto-detects DS/IS text vs FL/FD binary encoding without relying on the VR field (which dcmjs drops for bulkdata values), mutates the instance in place, and is wired into both the lazy and non-lazy metadata retrieval paths.getPTImageIdInstanceMetadatafixes a logic bug in the Philips-tag guard (the previous||chain was alwaystrue), addsfirstSequenceItemto handle both array and flattened-object sequence shapes, and coerces all numeric fields throughcoerceNumberso downstream consumers always see a finite number orundefined.Confidence Score: 5/5
Safe to merge; the worst-case failure mode (bulkdata fetch fails) leaves SUV factors absent rather than corrupted.
Both code paths (lazy and non-lazy) now resolve the Philips private tags before instances are registered. Errors are caught per-instance so a single failed fetch cannot block the entire load. The coerceNumber backstop in getPTImageIdInstanceMetadata ensures a BulkDataURI object can never silently corrupt calculate-suv output. Test coverage is thorough across DS text, FL/FD binary, error paths, and the non-PT guard.
No files require special attention; the performance note on resolvePETPrivateScalarBulkData is worth considering for very large series on HTTP/1.1 servers.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(pt): resolve Philips PET private SUV..." | Re-trigger Greptile