Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
draft
Model
to MTK.___System conversion. #40draft
Model
to MTK.___System conversion. #40Changes from 7 commits
cfe431e
fd6ad63
32f5e4d
5f94736
d188def
402acbc
6558506
de7d16b
bf6b080
f837186
f9221d1
09a7228
8e6a03c
7e58ca6
4c7e8dd
2c483c0
3347462
b235622
4da59f3
f7af82d
6b64f41
fb84639
a3dce09
570bc7c
beda38e
ec1fa52
6ee8876
bfe4170
40b1d98
ce4f03b
537296b
5455600
e686e91
6913135
a1af25a
1ffe96a
dce53c3
44d88f5
69fa2ff
3d0e09f
667240f
2d98356
42fcd07
d56ab94
b7d9abb
f528a2f
1909a19
3da6440
1202b30
a9a19af
f23b0c1
77a5743
e51201d
778ba4a
48db58b
3600346
9932b3b
7e11c2e
d714fb5
ea55421
b11f448
4169c47
87766de
f460328
59fd135
f726cdb
3c9d313
67a8f84
bbe19eb
4ae0332
1070601
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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'm not sure we will want to steal the String dispatch, since there are other formats that create a ReactionSystem (BioNetGen IIRC).
I think just having
ModelingToolkit.ReactionSystem(model::Model; kwargs...)
might be good enough.It's two function calls, but saves us from committing type piracy down the line.
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.
same for this dispatch, I think this is type-piracy https://docs.julialang.org/en/v1/manual/style-guide/#Avoid-type-piracy. It's not a big deal ATM though, so I'm fine with it
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.
Not a problem now, 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.
The real question is where reaction network importers like our SBML importer (but also CellML and BioNetGen importers) should go in the end. I am not yet convinced that having separate packages for every model specification language is the right way to go. What type of file is handled can be automatically inferred from the ending or header given a filename as String.
We could put the importers in:
String
to theReactionSystem
constructor:ModelingToolkit.ReactionNetwork(filename::String)
NetworkFileFormat
constructor such asBNGNetwork()
and filename toloadrxnnetwork
:ReactionNetworkImporters.loadrxnetwork(BNGNetwork(), filename)
@isaacsas: any thoughts?
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.
@shahriariravanian: what are your plans for CellMLToolkit. Do you think it should be moved under ReactionNetworkImporters.jl (see also Sam's and Chris' comment below)? Or shall it remain a standalone library?
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 think eventually it is a good idea to put all of them under the same package, but ReactionNetworkImporters doesn't seem like the right place for CellMLToolkit.
First, CellMLToolkit mainly outputs ODESystems (and possibly DAESystems in the next version), not reaction networks. While CellMLToolkit specification can handle reaction networks, very few actual CellML files contain one (the same that SBML files can define an ODE, but most don't). For whatever historical reason, CellML and SBML have diverged to their own niches.
Second, while it is currently only an importer, the future plan for CellMLToolkit is -- after adding DAEs and implementing units handling and Unitful integration -- to write an exporter. I don't know if you plan to add export functionality to SBML, but I feel that to use the full benefits of symbolic manipulation, we need to have the capability to output the results.
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.
@ChrisRackauckas @isaacsas @shahriariravanian @anandijain : After @shahriariravanian 's response that CellMLToolkit will grow bigger and does not fit well under the ReactionNetworkImporters umbrella I am inclined to have all Toolkits for biomodel importers as independent packages. Most users have their favourite model specification language and will ever only need one importer, so there are few reasons to combine them in one package. The only reason I can think of, is to make sure they have a similar API. But for the few users who use two or more model specification languages we can still take care that the API looks similar across importers.
To this end, I could imagine mimicking the constructors of the
MTK.AbstractSystem
types without overloading them to avoid type piracy. A bit like so (or is this a bad idea?):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.
Please don't shadow, just extend.
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.
To avoid misunderstandings, I assume shadowing is creating another function with the same name that takes precedence over the old function. And extending means overloading with a more specific type to make use of multiple dispatch. Is this correct?
Anyway, I do not get it to work. Especially not without avoiding type piracy.
Do you have any suggestions?
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.
regarding the docstrings, please include full function types and signatures, and preferably some hint about what precisely the function does it & how.
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.
you don't need a full copy here, just copy()ing the model struct and deepcopying the species is okay.
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.
Do we need to do this if
X + Y ↔ XY
is valid catalyst? https://catalyst.sciml.ai/stable/tutorials/basics/#Combining-several-reactions-in-one-lineAlternatively, it could be something that Catalyst needs. It seems like a useful transform to have.
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.
Maybe worth pinging Sam about
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.
The way as SBML would describe the kineticLaw for a reversible reaction is
X + Y ↔ XY
iskAs*X*Y-kDi*XY
. Catalyst however needskAs*X*Y, kDi*XY
, that is two kineticLaws. I mean this is a simple example, but somehow we need to find a way that generally works in splitting a combined kineticLaw in a forward and reverse kineticLaw. Any suggestions how to do this in a clean way?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.
IIRC @exaexa was taking this Math* sublanguage conversion. But I can get started on it for sure
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.
mutable
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.
Unfortunately it seems nature is not made of integers, at least not at this zoom level. :D
SBML allows full floats here and many models use that. (There's even functionality that uses additional denominators to write
0.33333333
precisely as1/3
, but let's pretend that one doesn't exist.)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.
In that case I think SBML allows stuff that nature doesn't (what on earth is a half-molecule?). But they sure have their reasons (e.g. some people might want to 1H + 0.5O -> 0.5*H2O). Thanks for the explanation, I was just puzzled.
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.
mutable
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.
Why have Maybe here? IMHO it may make some sense for the gene products (there might be a difference between "there are no gene products linked" vs "we don't talk about gene products here"), but certainly not for fundefs, if these are empty then they are just empty with no additional semantics right?
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.
Makes sense. Fixed with a1af25a. I also took Maybe out of gene_products, because I do not see how the difference between "there are no gene products linked" vs "we don't talk about gene products here" can be seen in the SBML file.
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.
The changes I made in a1af25a caused test cases to break. fixed in 44d88f5.
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.
?
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.
fixed with 667240f