-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Equations and events reupload #801
Conversation
f362a37
to
6e9bd40
Compare
0ab610e
to
a4cb3b9
Compare
a4cb3b9
to
1e97178
Compare
ext/CatalystHomotopyContinuationExtension/homotopy_continuation_extension.jl
Show resolved
Hide resolved
ext/CatalystHomotopyContinuationExtension/homotopy_continuation_extension.jl
Show resolved
Hide resolved
ext/CatalystHomotopyContinuationExtension/homotopy_continuation_extension.jl
Show resolved
Hide resolved
Figured I should have another go through and see if I missed anything, and was immediately punished. On the plus side I discovered another MTK bug: https://github.com/SciML/SciMLBase.jl/issues... |
Ok, I've updated to the new MTK version where this was fixed, all good now. |
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 can leave the reformats I didn't address as is. But please stop rewriting / refactoring perfectly fine existing code as part of PRs that are adding new functionality. It just makes the PR much longer, increasing the review time (it is much easier to find time here and there to review 10 200-line PRs than one 2000 line PR). Modifying existing code in ways that does not change what it is doing or fix a bug should be left for separate PRs.
ext/CatalystHomotopyContinuationExtension/homotopy_continuation_extension.jl
Show resolved
Hide resolved
src/reactionsystem.jl
Outdated
# Filters away any potential obervables from `states` and `spcs`. | ||
obs_vars = [obs_eq.lhs for obs_eq in observed] | ||
unknowns = filter(state -> !any(isequal(state, obs_var) for obs_var in obs_vars), unknowns) | ||
spcs = filter(spc -> !any(isequal(spc, obs_var) for obs_var in obs_vars), spcs) |
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 don't think we should take bad input and fix it, instead we should just error if a user gives something that is defined as an observable as a species or variable. (And maybe this should be a separate function to encapsulate the check.)
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.
(Part of the reason for this is that a user might be incorrectly understanding how observables and variables/species work, so it is better to error and let them correct their model I think.)
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 is needed because if you want to e.g. change whether an observable is a species/variable you do
@species O(t)
in the macro. However, then that observable is considered a species of the system (and have to be removed at the next stage).
If you want we can have an error check in the programmatic ReactionSystem
constructor that checks for this and throws an error.
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.
Then shouldn't this be part of the DSL? I still don't think that ReactionSystem
s should be "fixing" bad input but should instead just error.
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'd be willing to have this in the two-argument constructor, but I don't think it should be in the inner constructor. The inner constructor implicitly assumes the collection of unknowns and parameters are correct and doesn't do discovery.
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 will look at revamping where this is done. Generally (in agreement with our previous discussion) I think that stuff like this should be handled outside of the DSL.
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
I have tried to rebase #736 on master. Unfortunately, there have been too many changes, and it is not feasible anymore. This PR builds that one.
Creating events in the DSL
MTK does not currently have a wrapper around the
@continuous_events
or@discrete_events
options. However, we here use the same syntax as for programmatic description, but in abegin ... end
block, with one event on each line. If there is only a single event, it can just be given after@X_events
.Creating equations in the DSL
Equations use the
@equations
option. This utilises some MTK stuff, but the notation is similar as when creating stuff programmatically. Both differential and algebraic equations are supported.Parameters and variables used within equations must be declared as such The exception is if we have a differential equation on the form
where
sym
is a single symbol. Here,sym
is inferred to be a variable (it can still be declared in@variables
, if you e.g. want to attach metadata). In these cases,D
is inferred to be a differential with respect to the independent variable.It is also possible to create new differentials using the
@differentials
option:these can then be used in equations. Note that if a non-default differential is used, the
V
variable have to be decalred.Working with hybrid equations
These work like before, however, with the following additions:
structural_simplify
on the finalXSystem
. All relevant problem calls now have astructural_simpligy = false
kwarg. Setting this totrue
ensures thatstructural_simplify
(notcompelte
) is called on the internally createdODESystem
.D(...)
(whereD
is some differential) are automatically set to0
).There are a couple of things which are broken, awaiting fixes in MTK, but these are rather tangential, and the main functionality works well.