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

Change states to unknowns #785

Merged
merged 14 commits into from
Mar 18, 2024
Merged

Change states to unknowns #785

merged 14 commits into from
Mar 18, 2024

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Feb 13, 2024

Changes mentions of "states" to "unknowns".
Changes get_states (and similar functions) to get_unknowns (and similar functions.

I have tried to go through the full repo, and ensure that states such as in "steady state", or "the state of the system" are fine. Realistically, once all of this is over, I will probably have to read through the docs from start to finish to check that it reads consistently and nice. However, this should mostly be fine.

We cannot check whether this passes CI until MTK v9 is released. Also, CI will fail until we have figured SciML/ModelingToolkit.jl#2459 out.

We may/may not want to merge this into #757, but figured it would be easier to keep things somewhat separated for now.

@TorkelE TorkelE changed the base branch from master to Catalyst_version_14 February 13, 2024 03:53
@TorkelE
Copy link
Member Author

TorkelE commented Feb 15, 2024

I have fixed the all_unitless deprecation issue here as well (turning this to the PR that allows you to actually create ReactionSystems using the new MTK version).

Now, we do all(u == 1.0 for u in ModelingToolkit.get_unit([unknowns; ps; iv])) to check whether all of [unknowns; ps; iv] are unitless.

@isaacsas isaacsas mentioned this pull request Feb 28, 2024
47 tasks
@TorkelE
Copy link
Member Author

TorkelE commented Mar 15, 2024

I think this is ready now, tried to update the MTK thing. Tests fail, but at least Catalyst compiles. Once: #749 we should be able to look at this now.

I try to change "state" to "unknown" where it make sense. At some point when we are done with all of this, we'll have to go through from start to finish and ensure that we are consistent with what is a state, species, unknown, variable, and whatever.

@ChrisRackauckas
Copy link
Member

Tests fail though, it needs the complete calls.

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.

Should be ok to merge after addressing these minor comments. (I didn't check the changes within tests.)

docs/src/catalyst_functionality/network_analysis.md Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
src/networkapi.jl Outdated Show resolved Hide resolved
TorkelE and others added 8 commits March 18, 2024 10:13
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE
Copy link
Member Author

TorkelE commented Mar 18, 2024

Tests fail though, it needs the complete calls.

We plan to handle this in a separate PR (on way), this one is needed to actually compile Catalyst though, hence we are sorting it out first.

@isaacsas
Copy link
Member

Then just merge when this is done, irrespective of the tests failing.

@TorkelE TorkelE merged commit f1157a5 into Catalyst_version_14 Mar 18, 2024
1 of 3 checks passed
@TorkelE TorkelE deleted the states_to_unknowns branch June 8, 2024 18:27
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

3 participants