-
-
Notifications
You must be signed in to change notification settings - Fork 198
Feature/issue 2682 hcubature #2838
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
Feature/issue 2682 hcubature #2838
Conversation
|
@nhuurre Would you be free to look at this PR? Multi-dimensional (h-adaptive) integration isn't exactly a strong suit of mine! The code is based on the Julia implementation |
nhuurre
left a comment
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.
It's not like I'm an expert on numeric integration either! Or C++. But here's some comments anyway.
SteveBronder
left a comment
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.
Not sure if I have time to be the main reviewer but a few small comments
…ox, alloc, clean_GenzMalik, make_box
…FranziStan into feature/issue-2682-hcubature pull changes priority_queue.
|
@rok-cesnovar and @serban-nicusor-toptal (apologies for tagging both, not sure who is best here), I can't replicate the OpenCL failure (with I've tested with the I've tested using an Intel Integrated GPU, as well as three Nvidia GPUs (V100, A100, P100). Any suggestions on the next steps for trying to replicate? Are either of you able to access the CI machine itself to run the test manually and see if it replicates outside of the Jenkins run? |
|
Hey @andrjohns I do have non-root access to one of the Linux instances. |
Brilliant, thanks! Can you run the following using the And let me know if it passes or fails? |
|
Uh, the host OS does not have |
|
Oh of course, didn't think of that! Can you ty running it in the docker container (the gpu-cpp17 one)? |
Full logSteps I went through: Output not captured during piping to file: Note: This ran on jenkins node, I do not have a login into jenkins2, keep this in mind when running the tests maybe there's something on just one of the hosts. You can play around with their tags in the Jenkinsfile when testing this. PS: The OpenCL GPU Tests are running on the PS: For the sake of testing I created a job that runs in parallel on both hosts. Jenkinsfile usedIf we take a look at the results of the job, we can see that the same exact steps are failing on Please see below the GPU information on And for the |
|
Thanks for the thorough work @serban-nicusor-toptal, this is great! I'll update the |
|
@serban-nicusor-toptal I tried setting the constraints that you mentioned (changes here), but it still failed in the same way. Would you mind double-checking whether I've missed a constraint/label? |
|
Hey @andrjohns I think this is because ( can confirm it is, the code that ran was still with v100 ) This is a security feature in Jenkins to ensure no malicious Jenkinsfile code can be injected. |
|
@andrjohns it seems to have failed in Threading Tests now, does that have anything to do with the machine error we encountered in the OpenCL tests ? |
|
Hey @andrjohns, it seems you two solved the continuous integration test problem! Is this PR now ready for merging or is there something else to do for me? |
Perfect! I'll just revert the |
andrjohns
left a comment
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.
Ready to merge when tests pass. Thanks for sticking through the long journey!
Hm .... the mrrr_test seems again to have failed ... |
@serban-nicusor-toptal did we have a fix for changing the Jenkins host for the OpenCL tests? |
|
Hey @andrjohns I just merged the fix into develop, feel free to merge develop into this branch and it should be fine. |
Hey @andrjohns, @serban-nicusor-toptal, now, the mrrr Test seems to be successful, great! |
|
@Franzi2114 Can you merge in the latest develop again? It should pass CI now, and then I'll merge this (finally!) |
|
Made it! Sorry for the delay here @Franzi2114! |
Great!! Thanks a lot :) This was a long journey ^^ Then I will merge the develop branch into the 7-parameter-ddm-pdf branch so that the third part of that pr can go on. |
Summary
Here, we add the multidimensional integration routine hcubature. We need this feature for issue #2682. This is a split from PR #2822.
We add the following files:
stan/math/prim/functor/hcubature.hpp
Tests
We add the following files:
test/unit/math/prim/functor/hcubature_test.cpp
Side Effects
No.
Release notes
New multidimensional integration routine.
Checklist
Math issue Adding 7-Parameter Drift Diffusion Model (DDM) PDF with partial derivatives to Stan Math #2682
Copyright holder:
Franziska Henrich (franziska.henrich@psychologie.uni-freiburg.de)
Valentin Pratz (pratz@stud.uni-heidelberg.de)
Christoph Klauer (christoph.klauer@psychologie.uni-freiburg.de)
Raphael Hartmann (raphael.hartmann@staff.uni-marburg.de)
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested