-
Notifications
You must be signed in to change notification settings - Fork 172
Add function Math.nearestInteger #4495
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
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
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.
More though seems needed on how to obtain a useable expression variability of the function result.
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.
(Just restoring my Request changes after changing my mind back and forth.)
2ef733c
to
5c4e708
Compare
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 general idea is good, but when using it outside of functions it becomes messy.
It seems we need the annotation GenerateEvents=true
and update such functions in terms of variability, e.g., along the lines of modelica/ModelicaSpecification#3610
5c4e708
to
8fb0347
Compare
I removed the usage in block |
235f587
to
684aed4
Compare
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.
LGTM.
Combined with modelica/ModelicaSpecification#3610 it will be generally useful.
One could possibly add "away from zero" already in description as the preferred method depends, https://en.wikipedia.org/wiki/Rounding (But it may also be too long.)
862dc25
to
728fd63
Compare
Incorporated. Fell free to review this last change. @henrikt-ma may I kindly ask for a second review. Thanks. |
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 don't think this can be accepted until it is relying on language features of a released version of the specification. According to current semantics, the output of this is a non-discrete-time Integer
, which I'm pretty sure is not the intention.
Co-Authored-By: Thomas Beutlich <modelica@tbeu.de>
Co-authored-by: Hans Olsson <HansOlsson@users.noreply.github.com>
728fd63
to
be66842
Compare
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Cherry-picked from #3247 as proposed by #3247 (review).