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

Micromanager: switch to DataTools.parseDouble for double parsing #3253

Merged
merged 2 commits into from Oct 29, 2018

Conversation

melissalinkert
Copy link
Member

Backported from a private PR.

This automatically prevents NumberFormatExceptions for invalid double values and handles the case when the decimal separator in the file doesn't match the current locale's decimal separator.

To test, use curated/micromanager/pr-3253, which was copied from curated/micromanager/qa-4984/, but with 0.5, replaced by "0,5", in metadata.txt. Without this PR, showinf -nopix -omexml metadata.txt will throw:

Exception in thread "main" java.lang.NumberFormatException: For input string: "0,5"
	at sun.misc.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:2043)
	at sun.misc.FloatingDecimal.parseDouble(FloatingDecimal.java:110)
	at java.lang.Double.parseDouble(Double.java:538)
	at java.lang.Double.<init>(Double.java:608)
	at loci.formats.in.MicromanagerReader.parsePosition(MicromanagerReader.java:789)
	at loci.formats.in.MicromanagerReader.parsePosition(MicromanagerReader.java:469)
	at loci.formats.in.MicromanagerReader.initFile(MicromanagerReader.java:311)
	at loci.formats.FormatReader.setId(FormatReader.java:1395)
	at loci.formats.ImageReader.setId(ImageReader.java:842)
	at loci.formats.ReaderWrapper.setId(ReaderWrapper.java:650)
	at loci.formats.tools.ImageInfo.testRead(ImageInfo.java:1034)
	at loci.formats.tools.ImageInfo.main(ImageInfo.java:1120)

With this PR, the same test should successfully initialize the file and display OME-XML with PhysicalSizeX and PhysicalSizeY set to 0.5.

Builds and memo files should not be impacted.

This automatically prevents NumberFormatExceptions for invalid double
values and handles the case when the decimal separator in the file
doesn't match the current locale's decimal separator.
@sbesson
Copy link
Member

sbesson commented Oct 24, 2018

Closing/reopening to include #3251

@sbesson sbesson closed this Oct 24, 2018
@sbesson sbesson reopened this Oct 24, 2018
Copy link
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.

Barring an extra comment about further cleanup, generally reads fine.

}
catch (NumberFormatException e) { }
}
else if (key.equals("YPositionUm")) {
try {
Position p = positions.get(getCoreIndex());
p.positions[i][1] = new Double(value);
p.positions[i][1] = DataTools.parseDouble(value);
}
catch (NumberFormatException e) { }
Copy link
Member

Choose a reason for hiding this comment

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

With the usage of the DataTools API, could we further simplify this code and remove the NumberFormatException handling in the reader?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 59ed134

DataTools.parseDouble should catch NumberFormatException internally.
@dgault
Copy link
Member

dgault commented Oct 29, 2018

This change looks good and has no negative impact on existing datasets. Tested by modifying existing datasets to produce the NumberFormatException as described without the PR. With the PR included DataTools parsing handles the commas and numbers are parsed correctly without exception.

PR is good to merge

@dgault dgault merged commit 478c3ee into ome:develop Oct 29, 2018
@sbesson sbesson added this to the 6.0.0 milestone Oct 29, 2018
@melissalinkert melissalinkert deleted the mm-double-parsing-develop branch January 15, 2019 21:29
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

3 participants