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.

Sign up for GitHub

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

Jump to bottom

fix: create and solve initialization system in linearization_function #2676

Merged
merged 14 commits into from
May 26, 2024
Merged

fix: create and solve initialization system in linearization_function #2676

merged 14 commits into from
May 26, 2024

Conversation

AayushSabharwal
Copy link
Member

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

Add any other context about the problem here.

ChrisRackauckas
ChrisRackauckas reviewed Apr 29, 2024
View reviewed changes
src/systems/nonlinear/initializesystem.jl Outdated
Comment on lines 104 to 109
function generate_algebraic_initializesystem(sys::ODESystem;
u0map = Dict(),
name = nameof(sys),
guesses = Dict(), check_defguess = false,
default_dd_value = 0.0,
kwargs...)
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is a lot of a copy, maybe we can do this instead with some undocumented kwargs in the other?

@AayushSabharwal AayushSabharwal mentioned this pull request Apr 29, 2024
Closed
5 tasks
@AayushSabharwal
Copy link
Member Author

Requires SciML/ModelingToolkitStandardLibrary.jl#291

ChrisRackauckas
ChrisRackauckas reviewed Apr 30, 2024
View reviewed changes
src/systems/nonlinear/initializesystem.jl Outdated
end
end

pars = [parameters(sys); get_iv(sys)]
nleqs = [eqs_ics; get_initialization_eqs(sys); observed(sys)]
nleqs = if algebraic_only
eqs_ics
Copy link
Member

Choose a reason for hiding this comment

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

you always need the observed equations?

@AayushSabharwal AayushSabharwal force-pushed the as/linearization-initialization branch from 0654773 to 009a741 Compare April 30, 2024 15:27
This was referenced Apr 30, 2024
Closed
Merged
@AayushSabharwal AayushSabharwal force-pushed the as/linearization-initialization branch 2 times, most recently from 7e6688e to c2a7118 Compare May 6, 2024 08:39
baggepinnen
baggepinnen reviewed May 6, 2024
View reviewed changes
src/systems/abstractsystem.jl Show resolved Hide resolved
test/downstream/linearize.jl Outdated Show resolved Hide resolved
@AayushSabharwal AayushSabharwal force-pushed the as/linearization-initialization branch from 042abb3 to cc31bf3 Compare May 7, 2024 09:31
@ChrisRackauckas
Copy link
Member

Looks like there's still linearization tests failing?

@AayushSabharwal
Copy link
Member Author

Yeah that's the one I'm having trouble reproducing. It errors in tests, but not when I run the code block manually in the REPL.

@ChrisRackauckas
Copy link
Member

Avoid any redefinition. If I had to guess it could be JuliaLang/julia#52635 cropping back up. If you split the safetestset into different parts does it go away?

@AayushSabharwal
Copy link
Member Author

Nope, still errors even in it's own file and @safetestset, but runs fine copy-pasted into the REPL

@ChrisRackauckas
Copy link
Member

Start Julia with forced bounds checks.

@AayushSabharwal
Copy link
Member Author

Thanks! Found the bug.

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented May 8, 2024 •
edited
Loading

The test runs now, but it gives incorrect values. I'm not sure where it's going wrong.

EDIT: Never mind, fixed it as soon as I posted this

EDIT2: Another correctness issue

@AayushSabharwal AayushSabharwal force-pushed the as/linearization-initialization branch 3 times, most recently from 5fd53b8 to c3a2641 Compare May 13, 2024 15:14
@AayushSabharwal AayushSabharwal mentioned this pull request May 16, 2024
Merged
@AayushSabharwal AayushSabharwal mentioned this pull request May 21, 2024
Merged
5 tasks
@AayushSabharwal
fix: create and solve initialization problem in linearization_function
81c68fa
@AayushSabharwal
fix: fix namespacing of array variables using unknowns
9a98ffa
@AayushSabharwal
fix: respect u0map in InitializationProblem
97d6b27
@AayushSabharwal
fix: runtime dispatch in replace!
4a20e32
@AayushSabharwal
test: mark mtkparameters tests as no longer broken
c162e00
@AayushSabharwal
test: fix serialization test observed function expr
2c17819
@AayushSabharwal
feat: add utility function for obtaining defaults and guesses
c48891c
@AayushSabharwal
feat: add SII.observed support for AbstractSystem
9f531c4
@AayushSabharwal
refactor: make dependent and numeric portions singletons
da7c9fc
@AayushSabharwal
fix: properly initialize initialization problem in linearization_func…
6633065 
…tion
@AayushSabharwal
fix: improve performance of linearization
0ec683a
@AayushSabharwal
test: mark linearization dummy derivatives test as broken
556067c
@AayushSabharwal
build: upper bound SymbolicUtils to <1.6
d6befb7
@AayushSabharwal AayushSabharwal force-pushed the as/linearization-initialization branch from 3898fce to d6befb7 Compare May 22, 2024 05:52
@ChrisRackauckas
Copy link
Member

What's the state here?

@AayushSabharwal
Copy link
Member Author

InterfaceI doesn't pass because of the OrdinaryDiffEq stuff, but everything else is good (except the downstream tests marked as broken)

@ChrisRackauckas ChrisRackauckas merged commit e1befe0 into SciML:master May 26, 2024
19 of 24 checks passed
@ChrisRackauckas
Copy link
Member 

InitializationSystem Test: Test Failed at /home/runner/work/ModelingToolkit.jl/ModelingToolkit.jl/test/initializationsystem.jl:238
  Expression: sol.retcode == SciMLBase.ReturnCode.Unstable
   Evaluated: SciMLBase.ReturnCode.MaxIters == SciMLBase.ReturnCode.Unstable

@oscardssmith I thought this was handled?

@oscardssmith
Copy link
Contributor

so did I. Was this using the most recent version of SciMLBase?

@ChrisRackauckas
Copy link
Member

Make a PR that bumps it and let's see if master fails.

@AayushSabharwal AayushSabharwal deleted the as/linearization-initialization branch May 26, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baggepinnen baggepinnen baggepinnen approved these changes

@ChrisRackauckas ChrisRackauckas ChrisRackauckas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AayushSabharwal @ChrisRackauckas @oscardssmith @baggepinnen