Skip to content

Conversation

yewalenikhil65
Copy link
Contributor

from #395

@isaacsas
Copy link
Member

isaacsas commented Sep 8, 2021

@yewalenikhil65 since systems are the fundamental unit in ModelingToolkit, what about having subnetworks generate a vector of new ReactionSystems each containing the appropriate species, parameters and reactiosn? Then users can just reuse the functionality from your previous PR.

Also, perhaps it makes sense to add an intermediary function that takes reactioncomplexesmap and inverts it, i.e. builds a mapping (vector of vectors?) from each reaction to the complexes that participate in it? Then that can just be passed into subnetworks as an optional kwarg.

@yewalenikhil65
Copy link
Contributor Author

yewalenikhil65 commented Sep 8, 2021

@yewalenikhil65 since systems are the fundamental unit in ModelingToolkit, what about having subnetworks generate a vector of new ReactionSystems each containing the appropriate species, parameters and reactiosn? Then users can just reuse the functionality from your previous PR.

I did think about this earlier but am confused over parameters for the sub -reactionSystems, as we push each reaction into vector. How do we decide what are parameters for the subsystem we intend to create ? Rates for each reaction is a field of reaction but parameter doesnt seem to be the field. ( in earlier versions there was a .ps field for reaction, if i remember correctly)
Also I think it's important to mention here motivation of introducing these functions of linkage classes and subnetworks. I want to be able to join multiple reactions say in a particular subnetwork by removing intermediatte or shared complexes. These techniques are useful in reduction methods. This was the reason I made subnetworks as Vector{Vector{Reaction}} and not Vector{ReactionSystem} .
But I see merit in your point, and creating Vector{ReactionSystem} will be more beneficial as it will allow us to create small sub-matrices (complex stoich n incidence) of these individual subnetworks in the reduction process.

Also, perhaps it makes sense to add an intermediary function that takes reactioncomplexesmap and inverts it, i.e. builds a mapping (vector of vectors?) from each reaction to the complexes that participate in it? Then that can just be passed into subnetworks as an optional kwarg.

This i think is doable and will try ! In this case, I hope it's not required to export this intermediary function into Catalyst.jl

@isaacsas
Copy link
Member

isaacsas commented Sep 9, 2021

You can get the parameters within the rate as follows:

ps = parameters(network)
newps = Set{eltype(ps)}()
for rx in linkageclass   # pseudocode...
    Symbolics.get_variables!(newps, rx.rate, ps)
end
# newps will now have all the parameters used in reaction rates in the reactions of the linkclass

@yewalenikhil65
Copy link
Contributor Author

You can get the parameters within the rate as follows:

ps = parameters(network)
newps = Set{eltype(ps)}()
for rx in linkageclass   # pseudocode...
    Symbolics.get_variables!(newps, rx.rate, ps)
end
# newps will now have all the parameters used in reaction rates in the reactions of the linkclass

Cool 😀 I did find get_variables , but i was trying to use one from ModelingToolkit and i was using it wrong way. Thanks for this hint ! I will update these functions today

@isaacsas
Copy link
Member

isaacsas commented Sep 9, 2021

Yeah, there are a bunch of these types of useful helper functions but they aren't always prominent in the docs, and they tend to be split across MT, Symbolics, and SymbolicUtils...

adding intermediary function and updating `subnetworks` and `linkagedeficiency`
tweaking tests for updated `subnetworks` and `linkagedeficiency`
@yewalenikhil65
Copy link
Contributor Author

I decided to call the intermediary function inside the subnetworks function because it is defined for an individual linkage class, and not as kwarg for subnetworks

@isaacsas
Copy link
Member

Also, don't forget to add the two new functions to be exported, and mention them in the History.md file.

@isaacsas
Copy link
Member

isaacsas commented Sep 11, 2021

@yewalenikhil65 with the new subnetwork functionality we can now determine if a network is weakly reversible right? (Doesn't this just correspond to checking if each of the subnetworks is strongly connected?)

So we just generate the subnetwork graph, and then call LightGraphs.is_strongly_connected on it.

@yewalenikhil65
Copy link
Contributor Author

@yewalenikhil65 with the new subnetwork functionality we can now determine if a network is weakly reversible right? (Doesn't this just correspond to checking if each of the subnetworks is strongly connected?)

So we just generate the subnetwork graph, and then call LightGraphs.is_strongly_connected on it.

This one is on my mind and have to check it out. (Bdw, weakly reversible also may involve some subnetwork being cyclic so it wouldn't return true from is_strongly_connected . We may have to use is_cyclic type of function also to check this)

I will check out this LightGraphs function today. Bdw, Sorry for the delay in updating this PR as I am little busy with family stuff, so have only phone nearby. Hopefully today I will be able to do it.

@isaacsas
Copy link
Member

No worries; take your time!

This reference says weakly reversible is equivalent to each linkage class being strongly connected: https://core.ac.uk/download/pdf/191323175.pdf

@isaacsas
Copy link
Member

@yewalenikhil65 I'll tweak as I was suggesting since it is confusing.

@isaacsas
Copy link
Member

@yewalenikhil65 please make sure in the future you use the standard indentation of 4 characters. Looking at the PR locally it seems you indented the new functions by 3...

@isaacsas
Copy link
Member

LMK your thoughts on how I've modified it. This seems a reasonable tradeoff between performance vs. minimizing allocations I think.

For the future, please try to remember about using a default indentation spacing of 4 characters, or preserving what is used in the file, and try to write f(a, b) and not f(a ,b). In most editors you can configure the default tab-width to four characters. Thanks!

@yewalenikhil65
Copy link
Contributor Author

For the future, please try to remember about using a default indentation spacing of 4 characters, or preserving what is used in the file, and try to write f(a, b) and not f(a ,b). In most editors you can configure the default tab-width to four characters. Thanks!

Sorry for this really and getting back late ! I guess I should learn some good PR practices.

@isaacsas isaacsas merged commit f030de5 into SciML:master Sep 13, 2021
@yewalenikhil65
Copy link
Contributor Author

I think this is good to go now. Lesson learnt that using loops could be more efficient sometimes than readymade functions like vcat and unique as i was doing
Thanks sir 😀

@isaacsas
Copy link
Member

It all depends on context! Those functions are super useful and have their place, but it is good to always think about how much memory is getting allocated to ensure stuff works well on large networks too.

@isaacsas
Copy link
Member

Thanks again for putting this forward. We’re well on our way to a nice library of network analysis codes now I’d say.

@yewalenikhil65
Copy link
Contributor Author

yewalenikhil65 commented Sep 13, 2021

Thanks again for putting this forward. We’re well on our way to a nice library of network analysis codes now I’d say.

i will get back with is_reversible and is_weakly_reversible. I am still thinking that i read somewhere, that not only strongly connected graphs but cyclics also can be treated as is_weakly_reversible. WIll come back with a source for it

@isaacsas
Copy link
Member

Isn’t a cycle strongly connected since any node in it has a path to each other node?

@isaacsas
Copy link
Member

(A directed cycle…)

@yewalenikhil65
Copy link
Contributor Author

Isn’t a cycle strongly connected since any node in it has a path to each other node?

shifting discussion here #401

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.

2 participants