-
Notifications
You must be signed in to change notification settings - Fork 168
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
Add Spice3-specific infiniteTime constant #4481
base: master
Are you sure you want to change the base?
Conversation
Modelica/Electrical/Spice3.mo
Outdated
package Constants "Spice3-specific constants" | ||
extends Modelica.Icons.Package; | ||
|
||
constant Modelica.Units.SI.Time infiniteTime= 1e20 "Numerically safe very large time horizon (3e12 years)"; |
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 the vague "numerically safe", could you point to the specific problem this solves? E.g. something like "Very large time horizon (3e12 years) that avoids overflowing values in time computations"?
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.
Sure
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.
Done.
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.
This description still does not make sense as it doesn't explain which overflow.
It is an OpenModelica issue, and we cannot know which other libraries use similar time values.
Overflow is what you get when you add something to the "maximum representable finite floating-point number" ModelicaStandardLibrary/ModelicaServices/package.mo Lines 184 to 185 in 8e7876b
@HansOlsson if you know how to express that synthetically in a comment, that shouldn't be more than one or two lines, please go ahead. |
No, it is a MSL issue.
Why should other libraries be relevant? This PR defines a constant to be used exclusively for the Spice3 models. |
package Constants "Spice3-specific constants" | ||
extends Modelica.Icons.Package; | ||
|
||
constant Modelica.Units.SI.Time infiniteTime= 1e20 "Very large time horizon (3e12 years) avoiding overflow in time computations"; |
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.
While we are defining new (local) constants, what do you think about not enlarging the scope of the problem of the unfortunate (incorrect) naming of Modelica.Constants.inf
, and give this constant a more accurate name, e.g. hugeTime
or largeTime
or something like that, instead of infinity, which it is not?
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.
It's infinite for all practical purposes. Large or huge have no definite meaning, large with respect to what? Infinite time period and width means you'll never see a falling edge, so you'll get the rising edge only.
If you look at the parameter window and you see that both period and width are "infinite" it is clear that there will be no periodic behaviour if you leave those parameters to their default values. Which I guess was the original intent of the authors.
The problem with using Modelica.Constants.inf
here is not in the name, it's in the value.
But I guess we are over-discussing this issue here...
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.
Okay, then.
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 think it's clear that this model has an issue with Undefined Behaviour according to MLS 3.3, caused by an overflow in Twidth
from adding something to the largest representable number.
It looks to me like the proposed fix has small scope and is simple and correct.
That docstring can still be tweaked, but Hans has already picked up on this, and I've commented on maybe a better name for the constant, but aside of that 👍 from me (for what that is worth, as I have no official role here).
I (and as I understand @henrikt-ma ) don't think that is clear at all, and have tried to clarify that. Additionally, the risk with introducing such hacks in MSL due to issues in specific tools, is that they might spread to other domains - and suddenly we have a model where a large stop-time would actually makes sense and later see that the model starts misbehaving. That risk can be reduced by using a substantially larger value - similarly as the previous value for And if we say that tools may scale values as they see fit any value may potentially overflow. |
Sorry @casella I won't review that. I was never involved in the development of Spice3 and I have never worked with Spice3. |
I think there are even more compelling arguments, see my comment in #4479, and also in MLS #3591. To make a long story short, assuming that expressions are considered literally in Modelica models is a highly questionable assumption; therefore, any reasoning on overflow strictly based on that assumption is also questionable. IMHO that settles the matter about the fact that using the updated Modelica.Constants.inf in this model is a bad idea, from a conceptual point of view.
For the record, I question the definition of "hack" for this PR. There is nothing hackish in there, from a modelling point of view it makes perfect sense. And I (as well as others) think that there are also valid conceptual reasons why the use of the current Modelica.Constants.inf is really not a good idea. So, it's not a hack, it's a bug fix. I also think that the risk you fear of that large stop time to make sense is actually zero, given that the currently selected value for infiniteTime is 250 times the estimated life of the universe. Let's get real. Or should I say Real? 😉
@HansOlsson despite what I just wrote I have no problems changing the limit from 1e20 s to 1e60 s, if that is enough to get this PR through and close this argument. Please let me know.
No, I beg your pardon but this is a specious argument. Of course tools won't scale values in an arbitrary way, they will do that to avoid very large or very small values. Looking at the SI values of physical quantities (not just time!) in any system I can think of, excluding cosmological stuff, it is very hard to get values outside the 1e-15 - 1e15 range. And it is really impossible to get values outside the 1e-40 -1e40 range. That is nothing compared to the dynamic range of double-precision numbers, which is 1e-308 to 1e308. So there are huge margins to avoid overflows. We just need to steer clear from the boundary, as others have acknowledged, and to do that with a healthy margin, given the huge headroom allowed. Even if you are worried that infinite^2 and infinite^3 should still be representable, that would mean using 1e100. In fact, I still think 1e60 (the old value for Modelica.Constants.inf) was a very good one-fits-all compromise to practically (but very safely) represent infinity, as we don't have Inf (in the sense of IEEE 754) in Modelica. I don't know who set that specific number, but to me it was wise and made a lot of sense. It is 1e40 times larger the largest practical value of any reasonable physical variable in SI units, yet still leaving room for a scaling factor up to 1e15 (giving 1e75), followed by raising it up to the fourth power (that gives 1e300) while still leaving a healthy 1e8 (that's 100 million) margin before the gates of doom. I mean, who could ask for more? So, I am still convinced that we should have a "large but safe value" defined in the MSL, at the old value of 1e60. But that's another story and it's out of scope for this PR, which is specifically about setting a reasonable "infinite" time threshold for signal generators. Let's get this done first, I really can't see any problem with it. |
Me neither. The issue we are discussing here has nothing to do with electronic circuit modelling, it only involves a flexible pulse signal generator, that can degenerate to a ramp generator with some very large default values for the period and pulse width. Maybe you can still have a look a it.
Could be a good idea, but not for MSL 4.1.0. In any case I would try to ask their opinion, if possible. |
I would be in favor of defining generally two constants "near_small" and "near_inf" with magnitudes with a safety margin to Constants.small and Constants.inf, but small or large enough that they won't be used normally in models AND with values that indicate that these are not meaningful values, but numerically safe, e.g. |
I can sort of understand that, but:
|
@HansOlsson it's to point a human reader to the fact "that's a special value, not just an unusual choice" |
For me it just triggers the "price tag reflex", and i suspect someone might be trying to deceive me. 😜 |
Especially with "near" in its name, one would think it is near 1e100 (so a discounted 1e100 - get it now!). However, the main point is still that it is more numerical superstition than numerical analysis - and the reason for this discussion is just an error in one tool; taking time from more productive discussions. |
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.
To me this solution is sufficient.
Thank you @christiankral! The general solution to this problem will be discussed elsewhere. Can we get an agreement on this local solution just for Spice3 and move on? |
Fixes #4478