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.
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 1071 ode time varying #1072
Feature/issue 1071 ode time varying #1072
Changes from all commits
3249c62
945623b
95b483a
b520398
9d3bc04
d2758b5
e5f3f80
e3c11ea
7424806
e650714
9ae6b9c
0203000
2469213
d63f775
6da349e
ccc509f
aff1ddc
4fbafb7
f91d816
aca37e8
47aab06
ecd616c
8426183
87fa2c1
d9de61e
40d9503
bdff4df
3cac34e
a34b94c
0e14d80
fc5f23b
88804a9
5aa7a86
4397245
45c9a65
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 a suggestion: I like the type names
T_t0
andT_ts
, more thanT1
andT2
. I realize numbering types is consistent with what's done in Stan, but in the future, it would be nice to have something likeT_init
andT_parm
, orT_y0
andT_theta
.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.
I agree...and we both flavors floating around. Older code is usually numbered an newer is not...this also depends on who is writing the code. I am not sure if we have a convetion. I tried to stay consistent with the rest of the code.
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.
Fair enough. This doesn't need to be addressed in this PR. I'll bring it up on the discussion forum, and maybe raise an issue on the subject. This applies to a lot of functions in Stan.
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.
Any reason these need to change? It doesn't look like we need the
double
versions until much later.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.
we anyway need a dbl version and I was hoping that doing the checks on the double vector is faster. No?
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.
This looks fine to me. And yes, this should be faster because we're bringing a smaller object in memory.
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.
Will template deduction not work here? It looks like everything is specified well enough...
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.
... I am not sure what you mean? Can this be written simpler?
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.
Template deduction means that you don't need to specify the type of the object in
< >
, because the types are deduced from the constructor arguments. So yes, it makes the code a little simpler. But it doesn't always work and should be tested.I'm fine with the code as it is: it's safe and what's happening is clear.