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

Updates for upstream breaking changes #290

Merged
merged 9 commits into from
Jan 24, 2021
Merged

Updates for upstream breaking changes #290

merged 9 commits into from
Jan 24, 2021

Conversation

shashi
Copy link
Contributor

@shashi shashi commented Jan 9, 2021

needs SciML/ModelingToolkit.jl#720 and the latest release of SymbolicUtils.jl

@ChrisRackauckas
Copy link
Member

Should do the compats too?

@shashi
Copy link
Contributor Author

shashi commented Jan 19, 2021

latexify failed again?!

@isaacsas
Copy link
Member

Looks like you forgot to evaluate the second reaction network in the file before generating the third and fourth test?

@@ -13,7 +13,7 @@ Reexport = "189a3867-3050-52da-a836-e630ba90ab69"
Catlab = "0.7.3, 0.8, 0.9, 0.10"
Latexify = "0.13.5, 0.14"
MacroTools = "0.5.5"
ModelingToolkit = "4.4.1"
ModelingToolkit = "5"
Copy link
Member

Choose a reason for hiding this comment

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

Bump Catalyst too for a new release?

test/latexify.jl Outdated
\ce{ \varnothing &<=>[\frac{p_{a} \left( B\left( t \right) \right)^{n}}{k^{n} + \left( B\left( t \right) \right)^{n}}][d_{a}] A}\\
\ce{ \varnothing &<=>[p_{b}][d_{b}] B}\\
\ce{ 3 B &<=>[r_{a}][r_{b}] A}
\ce{ \varnothing &->[K1^{n1} v1^{2} \left( \mathrm{X4}\left( t \right) \right)^{n1} \mathrm{inv}\left( K1^{n1} + \left( \mathrm{X2}\left( t \right) \right)^{n1} \right) \mathrm{inv}\left( K1^{n1} + \left( \mathrm{X4}\left( t \right) \right)^{n1} \right)] X1}\\
Copy link
Member

Choose a reason for hiding this comment

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

This is for the wrong reaction network, first evaluate the network on line 78,

r = @reaction_network begin
    (hill(B, p_a, k, n), d_a), 0  A
    (p_b, d_b), 0  B
    (r_a, r_b), 3B  A
end p_a k n d_a p_b d_b r_a r_b

then generate this test and the last test.

@isaacsas isaacsas changed the title WIP: Updates for upstream breaking changes Updates for upstream breaking changes Jan 22, 2021
@isaacsas
Copy link
Member

If tests now pass I think this should be gtg.

@isaacsas isaacsas closed this Jan 22, 2021
@isaacsas isaacsas reopened this Jan 22, 2021
@isaacsas
Copy link
Member

@shashi seems like I'm now seeing crashes in a non-Latexify test. (The SDE test.) Does that pass for you?

@isaacsas isaacsas closed this Jan 22, 2021
@isaacsas isaacsas reopened this Jan 22, 2021
@isaacsas
Copy link
Member

@TorkelE We seem to have a reproducible test failure here with the latest ModelingToolkit and SymbolicUtils in the SDE tests. Could you take a look?

@isaacsas
Copy link
Member

FWIW, the test passes locally on my computer when I just include the SDE test file, but fails when I run test through the package manager.

@TorkelE
Copy link
Member

TorkelE commented Jan 23, 2021

Weird, bussy today, but will hopefully be able to take a look tomorrow.

@isaacsas
Copy link
Member

@TorkelE I dropped checking the solution in those SDE tests, and instead switched to just testing if the generated f and g functions agree well. Can you take a look and let me know if these changes look ok to you? From what I could tell the test had a bug anyway and was not actually comparing the two different solutions (instead looking at the ratio of a solution to itself).

@TorkelE
Copy link
Member

TorkelE commented Jan 24, 2021

I was a bit ambitious and figured that for a solution over a long-enough timeframe the means and stds would be similar enough, but if the f and g functions are identical there should be no problem (and if there is, it's unlikely to be in this package). Doing it like this is good to me, especially when it saves us some headache. And as you noted, the test was bugged anyway.

@isaacsas isaacsas merged commit e0bda0f into master Jan 24, 2021
@isaacsas isaacsas deleted the s/newmtk2 branch January 24, 2021 17:39
@isaacsas
Copy link
Member

@shashi finally done! Thanks for all your help migrating us over!

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.

4 participants