-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Inverse problem doc remake and new petab tutorial #701
Conversation
Worth noting is this problem noted in the last PEtab tutorial PR:| This tutorial is now in a good state where I am happy to receive feedback before merging. A side note (and a problem) is that I am unable to update the Project.toml file in the docs environment to use the latest PEtab version (2+). I get this error: (docs) pkg> update PEtab which does not directly seem to involve PEtab. Unsure how to interpret it, but I have had various weird package management errors relating to Sundials of late. |
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.
Looks good! Just some minor comments, primarily on adding small things to the documentation
1eb7bd5
to
f21f399
Compare
Co-authored-by: sebapersson <[email protected]>
Co-authored-by: sebapersson <[email protected]>
f21f399
to
5d9a1dd
Compare
@isaacsas Did you have a look at whether we could merge this one? It passes everything, so should be good to go. There is a couple of stuff which depends on the new doc structure, so it would save me (and Sophie) a decent amount of git effort to have this one merged asap. |
docs/src/catalyst_functionality/example_networks/basic_CRN_examples.md
Outdated
Show resolved
Hide resolved
docs/src/catalyst_functionality/example_networks/basic_CRN_examples.md
Outdated
Show resolved
Hide resolved
kD, SE --> S + E | ||
kP, SE --> P + E | ||
end | ||
``` |
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.
``` | |
rn = complete(rn) |
We might as well start converting over to using `complete` and symbolic indexing everywhere with all PRs we merge going forward.
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'd rather hold off on this for now, until I have figured out what the scope of these changes actually are.
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.
What do you mean by the scope?
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.
As in the scope of the changes to MTK and what they would mean for us.
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
I have not cut the identifiability thing (which I still prefer this way), and the symbolic usage (the same) I have made changes for everything. Still would need to go through to check that it builds properly and that I didn't miss any of your comments, but otherwise this should be good to go. |
Co-authored-by: Sam Isaacson <[email protected]>
Supersedes:
#674
#695
Since the inverse problem doc remake PR had the same git mess as the petab tutorial one (with the latter probably inheriting from the former). I merged them into one, new one.