Skip to content

Improved Symbolic Indexing #342

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
Merged
ChrisRackauckas merged 12 commits into from
Jan 26, 2023
Merged

Improved Symbolic Indexing #342

ChrisRackauckas merged 12 commits into from
Jan 26, 2023

Conversation

@AayushSabharwal
Copy link
Member

@AayushSabharwal AayushSabharwal commented Dec 12, 2022

Uses SymbolicIndexingInterface.jl
Refer to SciML/ModelingToolkit.jl#1988

@codecov
Copy link

codecov bot commented Dec 12, 2022 •
edited
Loading

Codecov Report

Merging #342 (acdb611) into master (c6a038e) will increase coverage by 1.15%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   57.75%   58.90%   +1.15%     
==========================================
  Files          43       44       +1     
  Lines        3290     3487     +197     
==========================================
+ Hits         1900     2054     +154     
- Misses       1390     1433      +43
Impacted Files Coverage Δ
src/SciMLBase.jl 75.00% <ø> (-6.82%) ⬇️
src/solutions/ode_solutions.jl 95.32% <85.71%> (-1.65%) ⬇️
src/integrator_interface.jl 47.38% <91.66%> (+0.20%) ⬆️
src/solutions/solution_interface.jl 59.72% <94.73%> (+<0.01%) ⬆️
src/scimlfunctions.jl 58.74% <100.00%> (+0.08%) ⬆️
src/debug.jl 0.00% <0.00%> (ø)
src/problems/basic_problems.jl 87.50% <0.00%> (+0.54%) ⬆️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

I'm fairly certain the rest of the tests will only pass once MTK gets bumped and this can depend on the new version

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

If paramsyms/indepsym is passed to the SciMLFunction, then I guess there isn't a reason to not fallback to it

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

This needs RecursiveArrayTools for tests to pass as well

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 12, 2022

Looks like it's still failing?

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

Yeah, I just realised that it needs the latest MTK, which is a bit backward and can be fixed by a small patch to the SymbolicIndexingInterface

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022 •
edited
Loading

I didn't realise that some functions contain the sys but may not use it for indexing, so it complains that they don't implement the interface. There's no way (as of now) to check if the interface is implemented, so it looks like defaulting is_state_sym(::Any, _) = false should do the trick

The compat change I just added should fix tests, but someone installing an old MTK would retain compat for the version containing this PR, and things would break.

@AayushSabharwal
Update SymbolicIndexingInterface compat, minor sym_to_index change
527a2ea 
- `sym_to_index` checks if the system has the specified symbol before
  falling back to `getsyms`. This takes advantage of the change in
  SymbolicIndexingInterface v0.2.0
@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

Now this should be compatible with previous MTK versions as well

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2022

🤦🏻 I forgot 0.1 - 0.2 is a breaking transition in semver, kept thinking it's just a minor version change

@oscardssmith oscardssmith mentioned this pull request Dec 12, 2022
Merged
oscardssmith pushed a commit to oscardssmith/ModelingToolkit.jl that referenced this pull request Dec 12, 2022
bump SymbolicIndexingInterface to 0.2
aa0ee12 
required for SciML/SciMLBase.jl#342
@oscardssmith oscardssmith mentioned this pull request Dec 12, 2022
Merged
@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 13, 2022 •
edited
Loading

Rerunning CI here should make everything pass 🤞🏻

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 13, 2022

Sounds suspicious.

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 13, 2022 •
edited
Loading

Is the new MTK not tagged?

Edit: Ah, it isn't. Then it'll run into this dependency conflict

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 13, 2022

Should I close this to not waste CI then reopen once its tagged?

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 13, 2022

Told you, sus.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 13, 2022

Just gotta wait a bit haha JuliaRegistries/General#74053

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 13, 2022

Dunno what is up with me today 😅 a bit all over the place

@ChrisRackauckas ChrisRackauckas mentioned this pull request Dec 14, 2022
Merged
@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 15, 2022 •
edited
Loading

Finally merged the new version of MTK

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 15, 2022

Apparently reopening doesn't rerun CI

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 15, 2022

Still failing

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 15, 2022

That failure confuses me. In the system at MTK/test/components.jl:99, why does states(sys) say that there are two states (capacitor.v and capacitor.i) but the solution is Nx1 (timeseries of one element)

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 15, 2022

Interestingly sol2.prob.f.syms has only capacitor.v. Must be some ODAEProblem thing I'm unaware of

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 15, 2022

The other must be an observed variable. How are those treated?

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 15, 2022 •
edited
Loading

If a symbol doesn't match a state variable, it falls back to using observed as it did before. However, the system says its a state variable, so it never falls back.

It seems ODAEProblem does some additional simplification, which seems backward to me. Why would a problem modify the system? In any case, it makes the information that the system has invalid, which causes problems

EDIT: Indeed, capacitor.i is turned into an observed variable, but the system does not know this

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 15, 2022

Might need to wait until @YingboMa is back

@ChrisRackauckas ChrisRackauckas mentioned this pull request Dec 28, 2022
Open
@YingboMa YingboMa mentioned this pull request Jan 3, 2023
Merged
@xtalax
Copy link
Member

xtalax commented Jan 26, 2023 •
edited
Loading

How is this going - this being merged seems like a prerequisite for symbolic save_idxs which I am working on.
SymbolicIndexingInterface seems to solve a lot of the same problems

@oscardssmith
Copy link
Member

oscardssmith commented Jan 26, 2023

it looks like this just needs a format

@ChrisRackauckas ChrisRackauckas merged commit 4f0fa68 into SciML:master Jan 26, 2023
@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Jan 26, 2023

Boom

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Jan 26, 2023

Is the ODAEProblem stuff fixed?

@YingboMa
Copy link
Member

YingboMa commented Jan 26, 2023

Yes

@oscardssmith
Copy link
Member

oscardssmith commented Feb 1, 2023

See #379 which makes this also work for DAESolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AayushSabharwal @ChrisRackauckas @xtalax @oscardssmith @YingboMa