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

Add Spice3-specific infiniteTime constant #4481

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

casella
Copy link
Contributor

@casella casella commented Oct 17, 2024

Fixes #4478

package Constants "Spice3-specific constants"
extends Modelica.Icons.Package;

constant Modelica.Units.SI.Time infiniteTime= 1e20 "Numerically safe very large time horizon (3e12 years)";

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@HansOlsson HansOlsson left a 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.

@casella
Copy link
Contributor Author

casella commented Oct 18, 2024

This description still does not make sense as it doesn't explain which overflow.

Overflow is what you get when you add something to the "maximum representable finite floating-point number"

final constant Real inf=1.7976931348623157E+308
"Maximum representable finite floating-point number";

@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.

@casella
Copy link
Contributor Author

casella commented Oct 18, 2024

It is an OpenModelica issue

No, it is a MSL issue. Modelica.Constants defines inf as the "Maximum representable finite floating-point number", and the Spice3 models add something to it, which doesn't make sense.

and we cannot know which other libraries use similar time values.

Why should other libraries be relevant? This PR defines a constant to be used exclusively for the Spice3 models.

@beutlich beutlich added the L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 label Oct 18, 2024
@beutlich beutlich changed the title Added Spice3-specific infiniteTime constant Add Spice3-specific infiniteTime constant Oct 18, 2024
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";

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?

Copy link
Contributor Author

@casella casella Oct 21, 2024

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...

Choose a reason for hiding this comment

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

Okay, then.

Copy link

@bilderbuchi bilderbuchi left a 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).

@HansOlsson
Copy link
Contributor

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.

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 Modelica.Constants.inf was 1e60.

And if we say that tools may scale values as they see fit any value may potentially overflow.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 24, 2024

Sorry @casella I won't review that. I was never involved in the development of Spice3 and I have never worked with Spice3.
Christoph Clauss and Kristin Majetta are no longer active.
I doubt that Spice3 is used a lot out there in the world. I would prefer to discontinue this sub-library.

@AHaumer AHaumer removed their request for review October 24, 2024 18:26
@casella
Copy link
Contributor Author

casella commented Oct 24, 2024

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.

I (and as I understand @henrikt-ma ) don't think that is clear at all, and have tried to clarify that.

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.

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.

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? 😉

That risk can be reduced by using a substantially larger value - similarly as the previous value for Modelica.Constants.inf was 1e60.

@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.

And if we say that tools may scale values as they see fit any value may potentially overflow.

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.

@casella
Copy link
Contributor Author

casella commented Oct 24, 2024

Sorry @casella I won't review that. I was never involved in the development of Spice3 and I have never worked with Spice3.

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.

Christoph Clauss and Kristin Majetta are no longer active. I doubt that Spice3 is used a lot out there in the world. I would prefer to discontinue this sub-library.

Could be a good idea, but not for MSL 4.1.0. In any case I would try to ask their opinion, if possible.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 27, 2024

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.
near_small = 9.999e-99
near_inf = 9.999e+99

@HansOlsson
Copy link
Contributor

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. near_small = 9.999e-99 near_inf = 9.999e+99

I can sort of understand that, but:

  • Just use 1e100 and 1e-100
  • I'm not sure where the small value would be used in practice
  • A safe large value depends on how it is used, the only actual numerical issue we've had with large values was some fluid model that cubed(?) a value that could be Modelica.Constants.inf (causing overflow). That would work with this value, but it is better to rewrite the model (as was done) and there might be another model using a higher power or something else. And on the other hand I recalled some bad code in a basic numerical routine that failed already for 1e4 a few years back (we worked around it in Dymola). The latter case gives an indication of what will be a real problem with such constants: as soon as we have them they are likely to remain and possibly even spread even long after the tool- or model-issues are corrected. Thus they will decrease understandability and traceability. Or basically it seems more like numerical superstition than good modeling.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 29, 2024

@HansOlsson it's to point a human reader to the fact "that's a special value, not just an unusual choice"
1e100 I would consider as larger but maybe meaningful value.
9.999e99 looks that weird that indicates "I'm a special value".
The small value could be used to indicate infinitely short rise time.

@bilderbuchi
Copy link

@HansOlsson it's to point a human reader to the fact "that's a special value, not just an unusual choice" 1e100 I would consider as larger but maybe meaningful value. 9.999e99 looks that weird that indicates "I'm a special value". The small value could be used to indicate infinitely short rise time.

For me it just triggers the "price tag reflex", and i suspect someone might be trying to deceive me. 😜

@HansOlsson
Copy link
Contributor

HansOlsson commented Oct 29, 2024

@HansOlsson it's to point a human reader to the fact "that's a special value, not just an unusual choice" 1e100 I would consider as larger but maybe meaningful value. 9.999e99 looks that weird that indicates "I'm a special value". The small value could be used to indicate infinitely short rise time.

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.

Copy link
Contributor

@christiankral christiankral left a 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.

@casella
Copy link
Contributor Author

casella commented Nov 11, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Questionable parametrization of V_pulse in Spice3 library leads to numerically unreliable computations
6 participants