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

Add event support mtkmodel #2427

Merged
merged 6 commits into from
Mar 11, 2024

Conversation

cgutsche
Copy link
Contributor

@cgutsche cgutsche commented Jan 17, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Here is the issue, related to this pr
Tests and documentation is added.

Since this is my first contribution, feel free to contact me, if i missed some guidlines or so.

@ChrisRackauckas ChrisRackauckas requested a review from ven-k January 17, 2024 09:58
Copy link
Member

@ven-k ven-k left a comment

Choose a reason for hiding this comment

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

The changes in src look good.
Test script needs a fix.

u0 = [model.x => 10, model.y => 0, model.z => 0]

prob = ODEProblem(model, u0, (0, 5.0))
sol = solve(prob, tstops = [1.5])
Copy link
Member

Choose a reason for hiding this comment

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

Default algorithm choices require DifferentialEquations.jl; the model_parsing.jl has only OrdinaryDiffEq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this by using Tsit5()

@@ -538,4 +568,4 @@ end

@test Equation[ternary_true.ternary_parameter_true ~ 0] == equations(ternary_true)
@test Equation[ternary_false.ternary_parameter_false ~ 0] == equations(ternary_false)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Format: missing new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird. In my local version there is an empty line. Git somehow deletes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now

@cgutsche
Copy link
Contributor Author

Hey, can you help me with those unsuccessful checks? Beside the spelling I do not really get why those are not working

@ven-k
Copy link
Member

ven-k commented Jan 18, 2024

  1. handeling -> handling for SpellCheck
  2. For formatting: Add JuliaFormatter to global env and then run format in MTK.jl dir.
Pkg.activate()
Pkg.add("JuliaFormatter")
format(".") # when pwd is MTK.jl dir

@cgutsche
Copy link
Contributor Author

I did the auto formatting, but maybe there was a problem with the last new line. Did it again, works now hopefully.

@cgutsche
Copy link
Contributor Author

cgutsche commented Feb 5, 2024

Sorry for not answering here for some time. I do not really get what those errors mean or why they occur. Can someone please have a look?

@ven-k
Copy link
Member

ven-k commented Feb 5, 2024

Dependent packages are being upgraded to v9; once they are ready, I'll retrigger the tests. The error is compat related and unrelated to changes introduced by this PR.

@ven-k
Copy link
Member

ven-k commented Mar 9, 2024

@cgutsche I can't push to your branch to resolve conflicts. I've pushed the changes to my branch; you can find changes here1 and here2.

Can you rebase this PR with current master?

Footnotes

  1. https://github.com/ven-k/ModelingToolkit.jl/blob/dev-add-event-support-mtkmodel/src/systems/model_parsing.jl

  2. https://github.com/ven-k/ModelingToolkit.jl/blob/dev-add-event-support-mtkmodel/docs/src/basics/MTKModel_Connector.md

@cgutsche cgutsche force-pushed the dev-add-event-support-mtkmodel branch from 645833e to a0d7454 Compare March 10, 2024 09:23
@cgutsche
Copy link
Contributor Author

Hey, thank you. I rebased.

@ven-k
Copy link
Member

ven-k commented Mar 11, 2024

I tried out this branch locally; tests pass. @YingboMa can you approve the workflow?

@ChrisRackauckas
Copy link
Member

I got it

@ChrisRackauckas
Copy link
Member

Needs formatting

@cgutsche
Copy link
Contributor Author

Formatting done ✅

@ChrisRackauckas ChrisRackauckas merged commit 5ac4469 into SciML:master Mar 11, 2024
17 of 24 checks passed
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.

3 participants