-
-
Notifications
You must be signed in to change notification settings - Fork 369
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
chainset::quantiles function catches exceptions raised by stan::math::quantiles when sample contains NaN's #3318
Conversation
It seems like the stan csv reader is using istream’s operator>> to parse numbers, which as far as I can tell is not defined for nan or infinite values. I’m guessing the reason we were witnessing something on MacOS only is that the different platforms do different things in this case. so:
|
It looks like Ubuntu doesn't throw these exceptions. |
It's because ubuntu is reading the |
I suspect changing stan/src/stan/io/stan_csv_reader.hpp Line 337 in 595ae41
to Not that this is performance critical, but I'd also think it would be faster (though there is a lot about the current code in that function that concerns me in that regard) |
just seeing this now. will change the CSV reader. |
one of the eight schools test files is triggering a std::out_of_range error from std::stod. indeed, the value is very close to zero - for some reason, there's an |
it seems using stold is able to parse that value, and then down casting to a double should work. The csv files were originally produced from double values, so I’m not worried about the down casting being lossy |
If the csv reader changes do work, I slightly preferred the rendition that caught logic_error to the current code that does a check that the quantiles function then repeats internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits but otherwise looks good!
Thanks so much for tracking this down!
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
changes made - I leave it to you to merge. |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Many thanks! |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
For samples where one or more draws is NaN, function
stan::math::quantiles
throws astd::invalid_argument
exception. Modifyquantile
andquantiles
functions so that they catch this, as well as thestd::domain_error
exception (thrown if the requested probability is outside of interval [0, 1]) and returnNaN
.Intended Effect
Prevent runtime exceptions being thrown by
stansummary
function.How to Verify
Unit tests.
Side Effects
N/A
Documentation
N/A
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: