New MR::to<std::complex>() specialisation#1794
Merged
jdtournier merged 3 commits intodevfrom Nov 20, 2019
Merged
Conversation
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).
jdtournier
approved these changes
Nov 20, 2019
Member
jdtournier
left a comment
There was a problem hiding this comment.
Looks pretty comprehensive to me...
I like your tests!
This was referenced Nov 20, 2019
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.
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
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.
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
mrcalcwhich means it needs to be robust.Of the 188 tests in
testing/cmd/testing_to.cpp, 56 currently fail onmaster/dev.Anyone feel free to add extra tests if you think you can make it misbehave.