Skip to content

Needed test updates #1250

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

Open
isaacsas opened this issue Apr 24, 2025 · 2 comments
Open

Needed test updates #1250

isaacsas opened this issue Apr 24, 2025 · 2 comments
Labels

Comments

@isaacsas
Copy link
Member

  1. We need to stop assuming ordering of unknowns/ps in tests. I tried to eliminate this in the main library long ago, but there are apparently still some tests that have issues in this regard.
  2. We should test that ps and us are subsets of system ps and us in general, and not test set equality (since MTK may now add extra stuff). When using structural_simplify we need to test vs. us and observed since us can be moved to observed.
  3. Some tests around conservation laws may be incorrect, see Aayush's Slack comment that:
https://github.com/SciML/Catalyst.jl/blob/master/test/upstream/mtk_structure_indexing.jl#L405
This test is incorrect. prob9 has the initial conditions Gamma[1] => 10.0, X1 => 10.0, X2 => 2.0 which are inconsistent since X1 + X2 ~ Gamma[1] (edited) 
[9:29](https://julialang.slack.com/archives/C0894545874/p1745501399505049)
The same holds for Gamma[2] => 20.0, Y1 => 3.0, Y2 => 4.0
@isaacsas isaacsas added the bug label Apr 24, 2025
@TorkelE
Copy link
Member

TorkelE commented Apr 24, 2025

  1. I think I did a long pass for this regarding inputs, but this seems to now become a thing regarding outputs as well. I will have a look at it.
  2. I'm trying to clarify this with Aayush. The tests are good I think, but we should do stuff to remove the warning as previously doiscussed.

@TorkelE
Copy link
Member

TorkelE commented Apr 24, 2025

The output thing is something we need to deal with more generally whenever structural_simplify has been involved. However, there are some implications for plotting I'd like to have ironed out first ideally, and I'd like to see what actually is going to happen in MTKv10 as well, as it feels like that is changing things up drastically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants