-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
[v14 - Ready] Fix bug where expand_registered_functions
mutated original reaction system
#929
Conversation
src/registered_functions.jl
Outdated
end | ||
return expr | ||
end | ||
# If applied to a Reaction, return a reaction with its rate modified. | ||
function expand_registered_functions(rx::Reaction) | ||
Reaction(expand_registered_functions(rx.rate), rx.substrates, rx.products, | ||
Reaction(expand_registered_functions!(deepcopy(rx.rate)), rx.substrates, rx.products, |
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.
Is this appropriate use of deepcopy
? I could not come up with a better way to do it. If we really want to avoid it it would be possible to first check the expression whether there are any registered functions, and only apply the function in this case
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.
Also, I think technically this by could be rewritten using @set
(since expanding functions really shouldn't change anything more of the reaction), however, I was reluctant to introduce any more changes than necessary.
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 would be more memory efficient to first check if the given expression would be expanded, and only then deepcopy and expand, but otherwise this is fine as a less memory efficient version.
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.
(Note, I'm not saying this needs to be done in this PR, just that it would be good to switch to that eventually.)
expand_registered_functions
mutated original reaction systemexpand_registered_functions
mutated original reaction system
src/registered_functions.jl
Outdated
end | ||
return expr | ||
end | ||
# If applied to a Reaction, return a reaction with its rate modified. | ||
function expand_registered_functions(rx::Reaction) | ||
Reaction(expand_registered_functions(rx.rate), rx.substrates, rx.products, | ||
Reaction(expand_registered_functions!(deepcopy(rx.rate)), rx.substrates, rx.products, |
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 would be more memory efficient to first check if the given expression would be expanded, and only then deepcopy and expand, but otherwise this is fine as a less memory efficient version.
Sorry, totally forgot about this one. I think I will add the checks, it should be fairly straightforward with the new symbolic functions. |
Ok, I have changed around the internals (no longer using |
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Previously
expand_registered_functions
would unintentionally apply the operation on the input system. E.g.would result in both
rs
andrs_expanded_funcs
having their functions expanded. This fixes this, and adds a test to ensure this remains the case.