Skip to content
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

Add ode_adjoint_tol_ctl #900

Merged
merged 20 commits into from
May 19, 2021
Merged

Add ode_adjoint_tol_ctl #900

merged 20 commits into from
May 19, 2021

Conversation

rok-cesnovar
Copy link
Member

Release notes

Added ode_adjoint_tol_ctl and improved type printing with variadic ODEs.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@rok-cesnovar rok-cesnovar mentioned this pull request May 16, 2021
Merged
5 tasks
@wds15
Copy link

wds15 commented May 16, 2021

Fyi: I will work on the doc during the week.

@wds15
Copy link

wds15 commented May 17, 2021

Do we have a volunteer to review this? Who did the reviews of the other variadic ODE bits for stanc3?

rok-cesnovar
rok-cesnovar commented May 17, 2021
View reviewed changes
src/middle/Semantic_error.ml
Stan_math_signatures.variadic_ode_fun_return_type
UnsizedType.pp arg0 UnsizedType.pp arg1 UnsizedType.pp arg2
UnsizedType.pp_fun_arg arg0 UnsizedType.pp_fun_arg arg1
UnsizedType.pp_fun_arg arg2
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is so the error also prints data for all variadic ODE types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO the signatures are pretty much unreadable either way but I suppose the data markers help if that was what caused the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree that the prints of the two large signatures is not great... But that is a different issue I guess as its not limited to just the variadic ODEs.

@rok-cesnovar
Copy link
Member Author

I think @nhuurre reviewed them mostly. If Niko does not have the time today or tomorrow, we can maybe ask @SteveBronder?

SteveBronder
SteveBronder reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

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

Apologies I tried giving this a review but I just don't think I know enough about these pieces of the compiler to understand the intuition behind a lot of the stuff going on here

src/frontend/Semantic_check.ml Outdated Show resolved Hide resolved
@wds15
Copy link

wds15 commented May 17, 2021

@SteveBronder do you think you can get this review sorted or should we ping a few more devs? This is the last thing to make adjoint ODE land in Stan.

@wds15
Copy link

wds15 commented May 17, 2021

@bbbales2 I started doc here: stan-dev/docs#358 not yet ready to merge, but it's a start.

@SteveBronder @nhuurre @rybern Anyone can complete this review before the freeze tomorrow on the 18th? This is the last thing to make the adjoint solver come to Stan.

@wds15
Copy link

wds15 commented May 18, 2021

@rok-cesnovar Do you think we can wait a little with the RC so that we get at least some feedback from stanc3 capable reviewers of this as to that they cannot make a review in time (or even wait until this is reviewed)? I would post on the forum and give people a heads up on this. Thoughts?

@rok-cesnovar
Copy link
Member Author

I am definitely fine with waiting a day or two given the lack of reviews(or reviewers) in the last few weeks across all the repos. I dont know how others feel about it though. I also have a few pull requests ready for about 10 days in cmdstan.

@wds15
Copy link

wds15 commented May 18, 2021

Ok, let's hope we find a reviewer for this in time. What cmdstan PR you want me to look at?

@wds15
Copy link

wds15 commented May 18, 2021

So I made a call on the forum... let's hope we find a solution soon. I'd actually suggest to possibly wait until the Thursday Stan meeting and make the decision there if to tag right after the meeting or if people are willing to review things within a few days. Looking at what is outstanding, I'd think it would be good to make that extra cycle of about 1 week (max).

@wds15
Copy link

wds15 commented May 18, 2021

arm-ec2 nodes are down? @serban-nicusor-toptal ?

@rok-cesnovar
Copy link
Member Author

It usually takes awhile to spin them up as we rarely need them. The tests have all passed which is the most important thing.

@wds15
Copy link

wds15 commented May 18, 2021

Great. Tests all pass. Review time.

nhuurre
nhuurre reviewed May 18, 2021
View reviewed changes
src/middle/Stan_math_signatures.ml
Comment on lines -99 to +116
[ (UnsizedType.AutoDiffable, UnsizedType.UReal)
; (AutoDiffable, UReal); (DataOnly, UInt) ]
[ (UnsizedType.DataOnly, UnsizedType.UReal)
; (DataOnly, UReal); (DataOnly, UInt) ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are for nonadjoint ODEs. Why did they change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bug currently. I should have mention it in the PR comment, sorry.

The tolerances in the nonadjoint should not allow autodiffable arguments as that causes C++ not to compile. This was reported by @bbbales2 in the Math PR but that PR has 500+ comments and I cant find the exact comment right now.

This would now error in stanc3 which is better I guess.

@nhuurre
Copy link
Collaborator

nhuurre commented May 18, 2021

GitHub doesn't let me make this a review comment because it's not something you added in this pull but

stanc3/src/middle/Semantic_error.ml

Lines 210 to 213 in cee75fa

if List.length args = 0 then
Fmt.pf ppf
"Ill-typed arguments supplied to function '%s'. Expected \
arguments:@[<h>%a@]\n\

This if is backwards. You're supposed to call generate_ode_sig when args nonempty, not when it's empty.

@rok-cesnovar
Copy link
Member Author

This if is backward

Nice find. Fixed.

@wds15
Copy link

wds15 commented May 18, 2021

Weird errors from test pipeline:

fatal error: file '/usr/include/x86_64-linux-gnu/sys/sysmacros.h' has been modified since the precompiled header 'stan/src/stan/model/model_header.hpp.gch' was built
note: please rebuild precompiled header 'stan/src/stan/model/model_header.hpp.gch'

@rok-cesnovar
Copy link
Member Author

Restart helped, green lights.

@rok-cesnovar rok-cesnovar merged commit 163bcb1 into master May 19, 2021
@rok-cesnovar rok-cesnovar deleted the ode_adjoint branch May 19, 2021 06:05
@wds15
Copy link

wds15 commented May 19, 2021

Hooray!!!! Thanks so much @nhuurre .... in particular for the short notice. Next time we give you an earlier heads-up for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveBronder SteveBronder SteveBronder left review comments

@nhuurre nhuurre nhuurre approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@rok-cesnovar @wds15 @nhuurre @SteveBronder