-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add function Math.nearestInteger #4495
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.)
Co-Authored-By: Thomas Beutlich <modelica@tbeu.de>
Co-authored-by: Hans Olsson <HansOlsson@users.noreply.github.com>
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.
Cherry-picked from #3247 as proposed by #3247 (review).