8000 [FIX] limit double precision to 17 digits during write by jpfeuffer · Pull Request #4636 · OpenMS/OpenMS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[FIX] limit double precision to 17 digits during write #4636

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 13, 2020
Merged

[FIX] limit double precision to 17 digits during write #4636

merged 4 commits into from
Apr 13, 2020

Conversation

jpfeuffer
Copy link
Contributor
@jpfeuffer jpfeuffer commented Apr 9, 2020 •
edited
Loading

fixes #4627

@ghost
Copy link
ghost commented Apr 9, 2020 •
edited by ghost
Loading

Congratulations 🎉. DeepCode analyzed your code in 1.169 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

@jpfeuffer jpfeuffer requested a review from cbielow April 9, 2020 11:21
timosachsenberg
timosachsenberg reviewed Apr 9, 2020
View reviewed changes
src/openms/include/OpenMS/DATASTRUCTURES/StringUtils.h Outdated
@@ -60,7 +60,9 @@ namespace OpenMS
class BK_PrecPolicy : public boost::spirit::karma::real_policies<T>
{
public:
static unsigned int precision(T) { return writtenDigits<T>(T()); }
static unsigned int precision(T n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • bracket in new line
  • add comment why this is sensible to do

@oliveralka
Copy link
Contributor

@jpfeuffer I probably test if that fixes the issue before merging or?

@jpfeuffer
Copy link
Contributor Author

Sure. If you have workspace ready where the test data is, it should be quick.

I wonder how many tests fail or if FuzzyDiff always catches the mini differences.

@timosachsenberg
Copy link
Contributor

in1: /home/travis/build/OpenMS/OpenMS/_build/src/tests/class_tests/openms/AccurateMassSearchEngine_test_319.tmp (line: 13, position/column: 14/36)
!
36310.1992

in2: /home/travis/build/OpenMS/OpenMS/src/tests/class_tests/openms/data/AccurateMassSearchEngine_output1.featureXML (line: 13, position/column: 14/36)
!
36310.199219

@jpfeuffer
Copy link
Contributor Author

Yes, so the problem is, we used our writtenDigits() function before which returns std::numeric_limits::digits10, which is (in my opinion) not what you want if you want to read
back in serialized doubles and guarantee that they will be the same.
I am happy about input here. https://www.boost.org/doc/libs/1_72_0/libs/multiprecision/doc/html/boost_multiprecision/tut/limits/constants.html gives a good overview of those two values.

@jpfeuffer
Copy link
Contributor Author
jpfeuffer commented Apr 9, 2020 •
edited
Loading

36310.1992

this is enough to represent the underlying single-precision float 36310.19921875 uniquely when reading it back in.

timosachsenberg
timosachsenberg reviewed Apr 9, 2020
View reviewed changes
src/openms/include/OpenMS/DATASTRUCTURES/StringUtils.h Outdated
// -> if scientific, subtract one
return BK_PrecPolicy<T>::floatfield(n) ?
std::numeric_limits<T>::max_digits10 - (floor(log10(n)) + 1) :
std::numeric_limits<T>::max_digits10 - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

why subtract one?
From the boost docu:
The extra two or three least-significant digits are 'noisy' and may be junk, but if you want to 'round-trip' - printing a value out as a decimal digit string and reading it back in - (most commonly during serialization and de-serialization) you must use os.precision(std::numeric_limits::max_digits10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for scientifc notation only.

@oliveralka
Copy link
Contributor

Let's see what the others think.

Tested locally in addition:

	 39 - DataValue_test (Failed)
	 62 - String_test (Failed)
	173 - MascotInfile_test (Failed)
	182 - MzMLFile_test (Failed)
	187 - MzTab_test (Failed)
	196 - ParamXMLFile_test (Failed)
	205 - SVOutStream_test (Failed)
	263 - DataFilters_test (Failed)
	388 - AccurateMassSearchEngine_test (Failed)
	442 - ItraqConstants_test (Failed)
	2221 - TOPP_OpenPepXL_1_out_2 (Failed)
	2226 - TOPP_OpenPepXLLF_1_out_2 (Failed)
	2286 - TOPP_FidoAdapter_1 (Failed)
	2287 - TOPP_FidoAdapter_1_out (Failed)
	2288 - TOPP_FidoAdapter_2 (Failed)
	2289 - TOPP_FidoAdapter_2_out (Failed)
	2290 - TOPP_FidoAdapter_3 (Failed)
	2291 - TOPP_FidoAdapter_3_out (Failed)
	2292 - TOPP_FidoAdapter_4 (Failed)
	2293 - TOPP_FidoAdapter_4_out (Failed)
	2294 - TOPP_FidoAdapter_5 (Failed)
	2295 - TOPP_FidoAdapter_5_out (Failed)
	2296 - TOPP_FidoAdapter_6 (Failed)
	2297 - TOPP_FidoAdapter_6_out (Failed)

@jpfeuffer
Copy link
Contributor Author

So, me and @timosachsenberg figured that we:
a) need an absolute value of the number first
b) that this formula will lead to longer strings for small values that do not yet undergo scientific writing mode (e.g. 0.001). Here it would ADD 4 to account for the first significant digit starting at position 4. Leading to an output of size 21, making it unreadable by the Java library.

So the solution to b) would be to switch to scientific mode starting from < 0.1

@timosachsenberg
Copy link
Contributor

if abs(N) < 0.1

@jpfeuffer jpfeuffer force-pushed the fix/doublePrecisionOutput branch from 2019e80 to b3c9772 Compare April 10, 2020 11:13
@timosachsenberg
Copy link
Contributor

did I get it right that the scientific notation has a fixed number of digits? And that lower number of written digits now works with luciphor?

@jpfeuffer
Copy link
Contributor Author

We set the precision to writtenDigits(), which is digits10 which is 15 for double.
Before, we allowed numbers from 0.0001 to 99999 to be written in fixed notation. Since boost karma
does not care about significant digits you still get an additional 15 digits after. 5+15 = 20 = too much.
If you switch to scientific starting at 10000 you get 1e05 plus the fractional part, so in total 16 digits.

@timosachsenberg timosachsenberg merged commit 366944c into develop Apr 13, 2020
@timosachsenberg timosachsenberg deleted the fix/doublePrecisionOutput branch April 13, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timosachsenberg timosachsenberg timosachsenberg left review comments

@cbielow cbielow Awaiting requested review from cbielow

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discussion] Unreasonable output precision of our new boost::karma generators?
3 participants
@jpfeuffer @oliveralka @timosachsenberg
0