-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Proposed FAq doc updates #874
Conversation
ebae997
to
f967bac
Compare
docs/src/faqs.md
Outdated
@@ -28,33 +26,35 @@ observed(osys) | |||
Let's solve the system and see how to index the solution using our symbolic | |||
variables | |||
```@example faq1 | |||
u0 = [rn.A => 1.0, rn.B => 2.0, rn.C => 0.0] | |||
ps = [rn.k₊ => 1.0, rn.k₋ => 1.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this rewriting, running thsi example takes crazy (infinite? I have a feeling that we have completed docs with it recently, so might finish, never bothered waiting that long in the retests though, but I have waited a good bit) long times, see this issue: #886
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing our docs can you make an MTK level example and open issue there so this can get fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the same issue occurs with ODESystems and is related to the new symbolic indexing stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now I am trying to figure out what is going on enough that I can actually make an MTK level issue. Given that I have over 50 unfixed MTK issues right now, I don't feel very confident that this will be fixed anytime soon, unfortunately. It was not my intention to simply change the docs so that the issue can be ignored (hence I raised the Catalyst issue of a starter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The change is only proposed, showing that some type of update is required here and what that could be, it is not meant as a final suggestion how the doc should be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this issue ever get resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's go with this version of the mappings then.
docs/src/faqs.md
Outdated
```@example faq1 | ||
```julia | ||
sol[osys.C] | ||
``` | ||
or | ||
```@example faq1 | ||
```julia | ||
@unpack C = osys | ||
sol[C] | ||
``` | ||
To evaluate `C` at specific times and plot it we can just do | ||
```@example faq1 | ||
```julia | ||
t = range(0.0, 10.0, length=101) | ||
plot(t, sol(t, idxs = C), label = "C(t)", xlabel = "t") | ||
``` | ||
If we want to get multiple variables we can just do | ||
```@example faq1 | ||
```julia | ||
@unpack A, B = osys | ||
sol(t, idxs = [A, B]) | ||
``` | ||
Plotting multiple variables using the SciML plot recipe can be achieved | ||
like | ||
```@example faq1 | ||
```julia | ||
plot(sol; idxs = [A, B]) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these no longer dynamic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to reduce runtime, sorry it got confused.
If you are happy with the change above I can reenable these, but if not we will have to figure something else out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you reenable these I am fine to merge. I'd prefer we keep everything dynamic if we can.
f967bac
to
44d539b
Compare
reuploaded due to a rebase issue