Skip to content

changing sciml base indexing and adding test #109

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

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

PavanChaggar
Copy link
Contributor

I was unable to symbolically index the solutions to differential equation solutions. I've changed SciMLBase.getsyms method to fix this and adding some test.

The plotting functionality is also maintained (lines are labelled with the correct symbols). I don't know how to test this though. It's still not possible to do something like plot(sol, vars=(:x,)).

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

Codecov Report

Merging #109 (08949fc) into master (b18a998) will increase coverage by 0.27%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   74.76%   75.04%   +0.27%     
==========================================
  Files          19       19              
  Lines         543      541       -2     
==========================================
  Hits          406      406              
+ Misses        137      135       -2     
Impacted Files Coverage Δ
src/compat/scimlbase.jl 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b18a998...08949fc. Read the comment docs.

@jonniedie
Copy link
Collaborator

Oh man, sorry to get back to you so late on this one. I don't know how I missed it.

@jonniedie
Copy link
Collaborator

I actually ended up reverting this because we need a way to handle nested ComponentArrays, which was the problem with this in the first place. Using keys only works for flat ComponentArrays. I think I'll have to add the ability to index by string with nested operations. And even then, I'm not sure this is going to work with the plot recipe right out of the gate. I suppose there is something to be said for at least getting this working with unnested ComponentArrays, though.

@jonniedie
Copy link
Collaborator

@PavanChaggar things should be good to go with now with cb798fc. The problem was is you have some nested variable like u.sys1.x, there wasn't a way to really access that with the plot recipe because it isn't a plain symbol. Now you'll be able to access them by wrapping the string in a Symbol like plot(sol; vars=Symbol.(["sys1.x", sys2.x"])). For non-nested variables, you can do it the normal way like plot(sol; vars=[:x])

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