Skip to content

fix getvar access of state #1824

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 20, 2022
Merged

fix getvar access of state #1824

merged 1 commit into from
Sep 20, 2022

Conversation

baggepinnen
Copy link
Contributor

The result of

i = findfirst(x -> getname(x) == name, sts)

right above the added code was never used, causing some states not to be found. I'm no sure if the added code is correct, but something is at least missing here

The result of 
```julia
i = findfirst(x -> getname(x) == name, sts)
```
right above the added code was never used, causing some states not to be found. I'm no sure if the added code is correct, but something is at least missing here
baggepinnen added a commit to baggepinnen/ModelingToolkitStandardLibrary.jl that referenced this pull request Sep 19, 2022
baggepinnen added a commit to baggepinnen/ModelingToolkitStandardLibrary.jl that referenced this pull request Sep 19, 2022
baggepinnen added a commit to baggepinnen/ModelingToolkitStandardLibrary.jl that referenced this pull request Sep 19, 2022
@@ -304,6 +304,9 @@ function getvar(sys::AbstractSystem, name::Symbol; namespace = false)

sts = get_states(sys)
Copy link
Member

Choose a reason for hiding this comment

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

These are already handled above. We should just remove this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an example that failed without this change but not with it. It's from the analysis point PR.
It might be related to how I added states to the system here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, we should remember to update the var_to_name map, when vars are updated.

Copy link
Member

@YingboMa YingboMa left a comment

Choose a reason for hiding this comment

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

We should remove the later state index check.

@YingboMa YingboMa merged commit 03d8b04 into master Sep 20, 2022
@YingboMa YingboMa deleted the baggepinnen-patch-1 branch September 20, 2022 16:20
@YingboMa
Copy link
Member

Let's merge this for now, but we should work on a more efficient solution later.

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.

2 participants