Skip to content

New MR::to<std::complex>() specialisation#1794

Merged
jdtournier merged 3 commits intodevfrom
complex_number_parsing
Nov 20, 2019
Merged

New MR::to<std::complex>() specialisation#1794
jdtournier merged 3 commits intodevfrom
complex_number_parsing

Conversation

@Lestropie
Copy link
Member

@Lestropie Lestropie commented Nov 8, 2019

Closes #1780.

This one dragged out for longer than was predicted. In the end I had to completely overhaul the string-to-complex conversion, because it's a custom format (a+bi) that the standard stream operator doesn't handle, and it's called by mrcalc which means it needs to be robust.

Of the 188 tests in testing/cmd/testing_to.cpp, 56 currently fail on master / dev.

Anyone feel free to add extra tests if you think you can make it misbehave.

A new algorithm for converting string data to complex numbers was necessary in order to make parsing of numerical input values in mrcalc usage more robust.
mrcalc will also now additionally give extra information whenever it is unable to interpret an input argument.
Explicit code in mrcalc for reading "nan", "-nan", "inf" and "-inf" was removed: the positive variants are handled appropriately using to<>(), whereas the latter are erroneously interpreted as command-line options prior to execution reaching this point.
A unit test for ensuring that the to<>() function does as expected in a wide range of scenarios was added.
- MR::strip() function removes carriage returns.
- Connectome LUT() Use MR::Strip() function.
The above two changes are necessary for correctly identifying the final column in a connectome lookup table as an integer if the file is saved using Windows newlines.
- testing_diff_matrix(): Support complex data (necessary to correctly parse and test mrstats for complex data input).
Copy link
Member

@jdtournier jdtournier left a comment

Choose a reason for hiding this comment

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

Looks pretty comprehensive to me...
I like your tests!

@jdtournier jdtournier merged commit daab5d3 into dev Nov 20, 2019
@jdtournier jdtournier deleted the complex_number_parsing branch November 20, 2019 12:40
Lestropie added a commit that referenced this pull request Nov 27, 2019
Due to the use of scientific notation in the definitions of default values for the number of permutations in these two commands, the floating-point version of template function get_option_value<>() was silently invoked, rather than the intended integer version. This meant that any value passed by the user at the command-line would be interpreted as a floating-point number. In cases where a decimal multiplier was specified (e.g. 1B = one billion), this would not be interpreted correctly, as such relies on parsing as an integer. Previously, this usage would silently result in a value of 1, since the trailing "B" would be silently ignored. The issue was detected due to #1794: such usage now throws an error since conversion of string "1B" to floating-point fails to consume the full string. This commit fixes the parsing of specifically these two command-line options.
Lestropie added a commit that referenced this pull request Jul 22, 2020
Prior change in the impelementation of strong FWE control, changing the command-line option from receiving a boolean to being a flag, was not propagated to the point in code at which that option was read. This however now results in an error upon attempting to use that option due to merge of #1794, as it erroneously attempts to convert the next item on the command-line following the -strong option to a double.
@Lestropie Lestropie mentioned this pull request Jul 1, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants