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.

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

Fix DynamicSelect for nfAPI #7891

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Conversation

perost
Copy link
Member

@perost perost commented Sep 14, 2021

  • Remove constant evaluation of DynamicSelect, since DynamicSelect is
    impure anyway and should never be constant evaluated.
  • Implement the same simplification rules for DynamicSelect that the OF
    uses:
    DynamicSelect("%y", String(y, significantDigits = 3)) => {"%y", y, 3}
    DynamicSelect(true, y) => {true, y}
    DynamicSelect(arg, ...) => arg

Fixes #5631

@perost perost added COMP/OMC/Frontend Issue and pull request related to the frontend COMP/OMC/Interactive Environment labels Sep 14, 2021
@perost perost self-assigned this Sep 14, 2021
@perost
Copy link
Member Author

perost commented Sep 14, 2021

With this fix the nfAPI should give almost identical output for DynamicSelect as the old API. The only real difference is that we handle this during simplification, at which point we don't know if the significantDigits argument to String was given by the user or from the default argument. But I guess always including significantDigits in the array doesn't matter.

@perost
Copy link
Member Author

perost commented Sep 14, 2021

I missed that there was already a test case for nfAPI, so I updated that one instead of changing another to use the nfAPI.

Copy link
Member

@adrpo adrpo left a comment

Choose a reason for hiding this comment

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

OK by me, small typo in commit message could be fixed "Dynamci"

@adrpo
Copy link
Member

adrpo commented Sep 14, 2021

Note that one might need to constant evaluate the first argument of DynamicSelect.

@perost
Copy link
Member Author

perost commented Sep 14, 2021

OK by me, small typo in commit message could be fixed "Dynamci"

Not sure where you see that, I can't find it.

Edit: Aha, you edit it.

Note that one might need to constant evaluate the first argument of DynamicSelect.

The first argument to DynamicSelect must be a literal value according to the specification. It seems we don't check that, but there shouldn't be any reason to constant evaluate it at least.

- Remove constant evaluation of DynamicSelect, since DynamicSelect is
  impure anyway and should never be constant evaluated.
- Implement the same simplification rules for DynamicSelect that the OF
  uses:
    DynamicSelect("%y", String(y, significantDigits = 3)) => {"%y", y, 3}
    DynamicSelect(true, y) => {true, y}
    DynamicSelect(arg, ...) => arg
@adrpo
Copy link
Member

adrpo commented Sep 14, 2021

Hm, I think I've seen cases where parameters or constants were used in the annotations.

@perost perost merged commit a5c325c into OpenModelica:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend COMP/OMC/Interactive Environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix DynamicSelect for new API
2 participants