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.

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

Add get_variables dispatch for Reaction #855

Merged
merged 4 commits into from
May 23, 2024
Merged

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented May 20, 2024

The get_variables function is currently defined for Equations but not Reactions. I've added it. It goes through all fields, checking and adds all symbolic variables to an output vector.

@TorkelE TorkelE changed the title init Add get_variables dispatch for Reaction May 20, 2024
Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

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

If we are going to add it we should probably add the in-place version and then just call that for the allocating version. It might also be worth using a Set and converting to a vector at the end instead. See https://github.com/JuliaSymbolics/Symbolics.jl/blob/1da13fdc375fccd6cd0a779a53b06be4ec7a54ec/src/utils.jl#L47-L79

@isaacsas
Copy link
Member

Actually, we already have an inplace version that takes Sets, so why not just wrap that and then convert to a vector at the end?

@TorkelE
Copy link
Member Author

TorkelE commented May 21, 2024

In practise, I just used what Symbolics define for Equations:

get_variables(eq::Equation) = unique(vcat(get_variables(eq.lhs), get_variables(eq.rhs)))

and I figured the above definition was most faithful to that.

Little bit uncertain exactly what rewrite you want, but if you have a different way to phrase the function content I am happy to update.

Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

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

A few minor changes. Once addressed and tests pass feel free to merge.

src/reaction.jl Outdated
append!(set, rx.substrates)
append!(set, rx.products)
for stoichs in (rx.substoich, rx.prodstoich), stoich in stoichs
get_variables!(set, stoich)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this check if stoich is a Symbolic? If not there is no reason to call get_variables!?

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, that is definitely an improvement.

src/reaction.jl Outdated Show resolved Hide resolved
src/reaction.jl Outdated Show resolved Hide resolved
@TorkelE TorkelE force-pushed the get_variables_for_reactions branch from 3f4fa76 to a55f782 Compare May 23, 2024 17:13
@TorkelE TorkelE merged commit f712acf into master May 23, 2024
5 of 6 checks passed
@TorkelE TorkelE deleted the get_variables_for_reactions branch May 23, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants