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

Sparse representation of stoichiometric matrices functions and matrices in complex-based functions #387

Merged
merged 10 commits into from
Aug 31, 2021

Conversation

yewalenikhil65
Copy link
Contributor

@yewalenikhil65 yewalenikhil65 commented Aug 26, 2021

from #386 (comment)

example. We use a second function argument of type Bool to switch sparsity on or off. Default is dense representation as earlier, i.e.sparsity=false

rn = @reaction_network begin
    k₁, 2A --> B
    k₂, A --> C
    k₃, C --> D
    k₄, B + D --> E
end k₁ k₂ k₃ k₄

julia> substoichmat(rn)   # usual function or substoichmat(rn,false)
4×5 Matrix{Int64}:
 2  0  0  0  0
 1  0  0  0  0
 0  0  1  0  0
 0  1  0  1  0

julia> substoichmat(rn,true)     # sparse representation
4×5 SparseMatrixCSC{Int64, Int64} with 5 stored entries:
 2        
 1        
     1    
   1    1  

adding SparseArrays dependency for sparse representations of `stoich` matrices
updating `stoichmat` functions and complex network functions for sparse representation
updating tests for sparse representation of `stoichmat` and complexesmat functions
update project.toml for sparsearrays
add using SparseArrays
@yewalenikhil65 yewalenikhil65 changed the title My catalyst Sparse representation of stoichiometric matrices functions and matrices in complex-based functions Aug 26, 2021
@yewalenikhil65
Copy link
Contributor Author

I think conservationlaws also needs a sparse version.

@isaacsas
Copy link
Member

@yewalenikhil65 have a deadline today for a report I need to submit -- I'll review both your PRs this weekend. (And yes, the conservation law methods should get a sparse version at some point too, but we can save that for a separate PR just to keep this easier to review.)

@yewalenikhil65
Copy link
Contributor Author

@yewalenikhil65 have a deadline today for a report I need to submit -- I'll review both your PRs this weekend. (And yes, the conservation law methods should get a sparse version at some point too, but we can save that for a separate PR just to keep this easier to review.)

No problem!

@isaacsas
Copy link
Member

@yewalenikhil65 take a look and let me know your thoughts on the changes. Mainly I made sparse=true a keyword to use for picking a sparse matrix. This is consistent with some of the usage in ModelingToolkit functions.

@isaacsas
Copy link
Member

I also should have made all matrices have columns = reactions now.

@isaacsas isaacsas merged commit e7ab5dc into SciML:master Aug 31, 2021
@yewalenikhil65
Copy link
Contributor Author

yewalenikhil65 commented Aug 31, 2021

@yewalenikhil65 take a look and let me know your thoughts on the changes. Mainly I made sparse=true a keyword to use for picking a sparse matrix. This is consistent with some of the usage in ModelingToolkit functions.

This looks nice, exactly how i imagined.. Thanks!
I was also little concerned of https://github.com/yewalenikhil65/Catalyst.jl/blob/6a2b79eafac8376586c5483491d8980d5da7dd17/src/networkapi.jl#L473
does transposing here add any considerable computational cost ? I remember conversation thread regarding SNF to be made optimized later or searching for alternative LinearAlgebra based methods

Anyway, i will get back to fixing PR In ReactionNetworkImporters after you release new Catalyst version

@@ -179,7 +179,7 @@ coefficient of the ith product within the jth reaction.
Note:
- Set sparse=true for a sparse matrix representation
"""
function prodstoichmat(::Type{SparseMatrixCSC{Int}}, rn::ReactionSystem; smap=speciesmap(rn))
function prodstoichmat(::Type{SparseMatrixCSC{Int,Int}}, rn::ReactionSystem; smap=speciesmap(rn))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it good idea to be using SparseMatrixCSC{Int,Int} entries ? i also remember some conversation thread in Catalyst issues to leave scope for non-integer stoichiometries later probablt in furture

Copy link
Member

Choose a reason for hiding this comment

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

For now it's fine. When we get generalized stoichiometry we'll have to think about what to do here. We don't want it to be type Any, so we may need some extra preprocessing to figure out the type, or we just error and don't allow these functions when there is symbolic stoichiometry.

@isaacsas
Copy link
Member

@yewalenikhil65 take a look and let me know your thoughts on the changes. Mainly I made sparse=true a keyword to use for picking a sparse matrix. This is consistent with some of the usage in ModelingToolkit functions.

This looks nice, exactly how i imagined.. Thanks!
I was also little concerned of https://github.com/yewalenikhil65/Catalyst.jl/blob/6a2b79eafac8376586c5483491d8980d5da7dd17/src/networkapi.jl#L473
does transposing here add any considerable computational cost ? I remember conversation thread regarding SNF to be made optimized later or searching for alternative LinearAlgebra based methods

Anyway, i will get back to fixing PR In ReactionNetworkImporters after you release new Catalyst version

I don't think this should be a big cost relative to computing the SNF, so using the transpose shouldn't be an issue. I think what @YingboMa was maybe suggesting is that we should implement and use the Bareiss method instead of SNF for finding the nullspace basis.

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