Skip to content

Use eps in a proper way in ComplexRampPhasor; it's relative. #4156

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

Closed

Conversation

HansOlsson
Copy link
Contributor

Based on #4042 (comment)

Note

  • This correction is needed regardless of Give a proper definition of machine constants. #4042
  • The factor 100 could be reduced (possibly down to 1) if desired.
  • Another possibility would be have an assert that duration>=eps*abs(startTime).
  • I don't understand the eps in the current assert-statement; so that hasn't been changed, but it might be an error.

@HansOlsson HansOlsson added the L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath label Jun 14, 2023
@HansOlsson HansOlsson changed the title Use eps in a proper way; it's relative. Use eps in a proper way in ComplexRampPhasor; it's relative. Jun 14, 2023
@HansOlsson HansOlsson added the bug Critical/severe issue label Sep 12, 2023
@Harisankar-Allimangalath
Copy link
Contributor

@AHaumer and @christiankral can you please review the initial changes and give your opinion , Thanks

@christiankral
Copy link
Contributor

As far as I remember, the intention was to avoid a step in the ramp block. For this purpose, in all calculations max(duration,eps) was used instead of duration. Therefore, if we replace eps by 100*eps*abs(startTime) the original intention to avoid steps would be overruled in case of startTime = 0.

I see two additional solutions here:

  1. I wonder if there really is a strong need to limit the slope of the output. So we could consider to remove the eps and just use duration instead of max(duration,eps). This is non-backwards compatible change for the very unlikely case of duaration<eps. However, a poper documentation of the applied changes of the model should more or less overcome this case (as 100*eps*abs(startTime) is not a backwards compatible change either).
  2. We define a constant or final parameter final parameter Modelica.Units.SI.Time TRef = 1; and use eps*TRef or 100*eps*TRef instead.

@christiankral
Copy link
Contributor

christiankral commented Dec 4, 2023

There is certainly no need for eps in the assert. I agree, this is an error. The assert shall be:

  assert(not useLogRamp or (magnitude1>0 and magnitude2>0),
    "ComplexRampPhasor: magnitude1 and magnitude2 have to be greater than zero, if useLogRamp = true");

@HansOlsson
Copy link
Contributor Author

As far as I remember, the intention was to avoid a step in the ramp block. For this purpose, in all calculations max(duration,eps) was used instead of duration. Therefore, if we replace eps by 100*eps*abs(startTime) the original intention to avoid steps would be overruled in case of startTime = 0.

Ah, yes.
I was thinking about the issue if startTime was large, where it failed to guard against that (see below), - but didn't consider what happens when it is very small (and duration is zero).

That could be handled using something like max(duration, eps*(max(abs(startTime), 1e-10))). (or break logic and use eps instead of 1e-10?) But it isn't nice.

I see two additional solutions here:

  1. I wonder if there really is a strong need to limit the slope of the output. So we could consider to remove the eps and just use duration instead of max(duration,eps). This is non-backwards compatible change for the very unlikely case of duaration<eps. However, a poper documentation of the applied changes of the model should more or less overcome this case (as 100*eps*abs(startTime) is not a backwards compatible change either).

That seems like the simplest solution. I tried running the model with that change, and it just worked.
The non-backwards compatible change is only in that short ill-defined region if duration is too short.

  1. We define a constant or final parameter final parameter Modelica.Units.SI.Time TRef = 1; and use eps*TRef or 100*eps*TRef instead.

That doesn't work well, if startTime is 1000s and duration is zero the eps will be lost in under-flow (as if the guard didn't exist). Having it as a non-final parameter would require people to set it that wouldn't be nice either.

@HansOlsson
Copy link
Contributor Author

Oh, looking at the documentation for duration it already says that duration=0 gives a step.

@HansOlsson
Copy link
Contributor Author

HansOlsson commented Dec 4, 2023

Oh, looking at the documentation for duration it already says that duration=0 gives a step.

However, the example Modelica.Magnetic.QuasiStatic.FluxTubes.Examples.BasicExamples will have infinite derivatives if:

  • We set duration=0 and startTime=1000 for the old model (and simulate past 1000s).
  • We set duration=0 and just use duration without any guard

So, we might as well skip the guard.
See: #4239

@henrikt-ma
Copy link
Contributor

Oh, looking at the documentation for duration it already says that duration=0 gives a step.

Just have a look at the amount of discussion in #4121 to see how problematic such a simple statement could be. There's a clear risk that the author of the documented behavior didn't realize what sort of complexity this brings to the model. Maybe it is time to also stop misusing ramps in the complex for steps?

HansOlsson added a commit to HansOlsson/Modelica that referenced this pull request Dec 11, 2023
@GallLeo
Copy link
Contributor

GallLeo commented Dec 12, 2023

MAP-LIB Group during monthly meeting:

#4239 is the cleaner proposal (without eps).
Close this one.

@henrikt-ma
Copy link
Contributor

Related PR with discussion about continuity of the Ramp source block: #4121

dietmarw pushed a commit to HansOlsson/Modelica that referenced this pull request Dec 13, 2023
@dietmarw dietmarw force-pushed the EpsInComplexRampPhasor branch from c8d3500 to d1abddd Compare December 13, 2023 10:46
@HansOlsson
Copy link
Contributor Author

Since #4239 is merged we should skip this one.

@HansOlsson HansOlsson closed this Dec 13, 2023
@beutlich beutlich added this to the never milestone Dec 13, 2023
@beutlich beutlich added the invalid Invalid issue label Dec 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Critical/severe issue invalid Invalid issue L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants