-
Notifications
You must be signed in to change notification settings - Fork 0
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
Generate adjoint model for simple discrete-time systems #11
Conversation
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.
Looks good - one likely unimportant question about `==
@@ -10,12 +10,23 @@ generate_dust_sexp <- function(expr, dat, options = list()) { | |||
ret <- sprintf("-%s", args[[1]]) | |||
} else if (n == 1 && fn == "+") { | |||
ret <- args[[1]] | |||
} else if (n == 2 && fn %in% c("+", "-", "*", "/")) { | |||
} else if (n == 2 && fn %in% c("+", "-", "*", "/", "==")) { |
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.
Just to check, are we happy here with ==
re the usual floating point comparison story?
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.
yeah, this is subject to the usual issues, but we don't try and be clever here (this has been in odin1 forever and works quite well in practice)
This PR depends on
Probably best to review and merge after #12 because that'll make getting an integration test in much easierIt is currently based off of #9 so that the scale of the PR is clearer -it's not that long, but it's all killer no filler.The testing is not amazing, but it's a start. The integration test here is doing a lot of heavy lifting, combined with keeping track of the generated code.