Skip to content

DICOM: fix width of values with DS VR#4379

Merged
sbesson merged 3 commits into
ome:developfrom
melissalinkert:gh-4376
Mar 17, 2026
Merged

DICOM: fix width of values with DS VR#4379
sbesson merged 3 commits into
ome:developfrom
melissalinkert:gh-4376

Conversation

@melissalinkert
Copy link
Copy Markdown
Member

Fixes #4376.

With this change, bfconvert -noflat -compression JPEG CMU-1.ndpi CMU-1.dcm or similar and then dcdump CMU-1_0_0.dcm should show:

...
  > (0x0028,0x0030) DS Pixel Spacing 	 VR=<DS>   VL=<0x0022>  <0.00045641259698\0.00045506257110 > 
...

In addition to the issue noted in #4376, this checks other values of type DS to make sure the width is correct. I don't necessarily like the current implementation of formatFixedWidth, so definitely open to better ideas for how to turn a double into a string of no more than n characters total. DecimalFormat and String.format(...) can easily give a string with at least n characters, or with a fixed precision, but I couldn't find a good concise way to guarantee no more than n characters.

For Pixel Spacing, the fixed width is set to 15 so that even if the padding space were included, the value will still have no more than 16 characters.

@fedorov
Copy link
Copy Markdown

fedorov commented Oct 28, 2025

...
  > (0x0028,0x0030) DS Pixel Spacing 	 VR=<DS>   VL=<0x0022>  <0.00045641259698\0.00045506257110 > 
...

Why not use scientific notation?

@fedorov
Copy link
Copy Markdown

fedorov commented Oct 28, 2025

Here's somewhat related issue, just for the reference - we went quite a bit back and forth on how to ensure best use of the limited character length: QIICR/dcmqi#414 (that code is C++).

@dclunie
Copy link
Copy Markdown

dclunie commented Oct 30, 2025

... I couldn't find a good concise way to guarantee no more than n characters.

You may want to look at my Java code, which handles a bunch of fringe conditions to try to maximize the preserved precision, in this method https://www.dclunie.com/pixelmed/software/javadoc/com/pixelmed/utils/FloatFormatter.html#toStringOfFixedMaximumLength-double-int-boolean-java.util.Locale-

The class source is attached for your convenience:

FloatFormatter.java

@fedorov
Copy link
Copy Markdown

fedorov commented Oct 30, 2025

Here's that function in a GitHub repo where I synchronize @dclunie pixelmed, for convenience:

https://github.com/fedorov/pixelmed/blob/f9b7844749f15fd8d6523d119f4c3792738c40a3/com/pixelmed/utils/FloatFormatter.java#L105-L227

@melissalinkert melissalinkert modified the milestones: 8.4.0, 8.5.0 Jan 12, 2026
Copy link
Copy Markdown
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

I was able to reproduce the issue using Bio-Formats 8.4.0 and CMU-1.npi and the dicom3tools utility

sbesson@Sebastien-GS-MacBook-Pro-2025 Downloads % BF_CP=libjpeg-turbo-java-0.1.0-SNAPSHOT.jar ./bftools/bfconvert -noflat -precompressed CMU-1.ndpi CMU-1.dcm  
CMU-1.ndpi
NDPIReader initializing CMU-1.ndpi
Reading IFDs
Populating metadata
Populating OME metadata
Implicitly using compression = JPEG
[Hamamatsu NDPI] -> CMU-1.dcm [DICOM]
More than 4GB of pixel data, compression will need to be used
Tile size = 2048 x 512
	Series 0: converted 1/1 planes (100%)
Tile size = 512 x 512
	Series 0: converted 1/1 planes (100%)
Tile size = 128 x 128
	Series 0: converted 1/1 planes (100%)
Tile size = 800 x 596
	Series 0: converted 1/1 planes (100%)
Tile size = 1191 x 408
	Series 1: converted 1/1 planes (100%)
[done]
22.121s elapsed (1.8+4146.6ms per plane, 711ms overhead)
sbesson@Sebastien-GS-MacBook-Pro-2025 Downloads % ./dicom3tools_macexe_1.00.snapshot.20250715101647/dciodvfy CMU-1_0_0.dcm 
Warning - NumberOfFrames does not match expected value for tiled total pixel matrix got 23.3683 expected 23.2992 for 51200 columns, 0.000455063 PixelSpacing between columns
Warning - NumberOfFrames does not match expected value for tiled total pixel matrix got 17.3579 expected 17.4094 for 38144 rows, 0.000456413 PixelSpacing between rows
Warning - Missing attribute or value that would be needed to build DICOMDIR - Patient ID
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study Date
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study Time
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study ID
Error - Value invalid for this VR - (0x0028,0x0030) DS Pixel Spacing  DS [1] = <4.564125969876768E-4> - Length invalid for this VR = 20, expected <= 16
Error - Value invalid for this VR - (0x0028,0x0030) DS Pixel Spacing  DS [2] = <4.550625711035267E-4> - Length invalid for this VR = 20, expected <= 16
Error - Dicom dataset contains invalid data values for Value Representations
VLWholeSlideMicroscopyImage

With this change included, the same conversion yields no Error when running dcdump

sbesson@Sebastien-GS-MacBook-Pro-2025 Downloads % BF_CP=libjpeg-turbo-java-0.1.0-SNAPSHOT.jar ~/Documents/GitHub/bioformats/tools/bfconvert -noflat -precompressed CMU-1.ndpi CMU-1.dcm 
CMU-1.ndpi
NDPIReader initializing CMU-1.ndpi
Reading IFDs
Populating metadata
Populating OME metadata
Implicitly using compression = JPEG
[Hamamatsu NDPI] -> CMU-1.dcm [DICOM]
More than 4GB of pixel data, compression will need to be used
Tile size = 2048 x 512
	Series 0: converted 1/1 planes (100%)
Tile size = 512 x 512
	Series 0: converted 1/1 planes (100%)
Tile size = 128 x 128
	Series 0: converted 1/1 planes (100%)
Tile size = 800 x 596
	Series 0: converted 1/1 planes (100%)
Tile size = 1191 x 408
	Series 1: converted 1/1 planes (100%)
[done]
18.958s elapsed (1.4+3613.4ms per plane, 509ms overhead)
sbesson@Sebastien-GS-MacBook-Pro-2025 Downloads % ./dicom3tools_macexe_1.00.snapshot.20250715101647/dciodvfy CMU-1_0_0.dcm 
Warning - NumberOfFrames does not match expected value for tiled total pixel matrix got 23.3683 expected 23.2992 for 51200 columns, 0.000455063 PixelSpacing between columns
Warning - NumberOfFrames does not match expected value for tiled total pixel matrix got 17.3579 expected 17.4094 for 38144 rows, 0.000456413 PixelSpacing between rows
Warning - Missing attribute or value that would be needed to build DICOMDIR - Patient ID
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study Date
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study Time
Warning - Missing attribute or value that would be needed to build DICOMDIR - Study ID
VLWholeSlideMicroscopyImage

and the output of dcdump CMU-1_0_0.dcm shows

    > (0x0028,0x0030) DS Pixel Spacing 	 VR=<DS>   VL=<0x001e>  <.00045641259699\.0004550625711> 

The float formatting API and the associated unit tests are consistent with the implementation suggested in the review and should protect against future regressions.

@sbesson sbesson merged commit d51c6f8 into ome:develop Mar 17, 2026
22 checks passed
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.

bfconvert to DICOM writes too much on a DS ValueType field

4 participants