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

Implement continuous version of SignalExtrema #4015

Merged
merged 9 commits into from
Nov 27, 2022

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented Jul 29, 2022

Triggered by a discussion with @GallLeo and looking into #3762, I developed a continuous block to detect signal extrema, based on derivative. It works for non-differentiable inputs, too.

@AHaumer AHaumer added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks labels Jul 29, 2022
@AHaumer AHaumer self-assigned this Jul 29, 2022
@AHaumer AHaumer requested a review from HansOlsson July 29, 2022 10:54
Modelica/Blocks/Math.mo Outdated Show resolved Hide resolved
@dietmarw dietmarw removed their request for review September 5, 2022 13:02
@christiankral
Copy link
Contributor

christiankral commented Sep 6, 2022

@dietmarw @AHaumer We had a discussion in #3762 (comment) regarding whether or not the name shall contain Continuous or Sampled not. I think we are back to this point. One simple way out is to move the models:

  • Modelica.Blocks.Math.ContinuousSignalExtrema => Modelica.Blocks.Math.Continuous.SignalExtrema
  • Modelica.Blocks.Math.SignalExtrema => Modelica.Blocks.Math.Discrete.SignalExtrema

@dietmarw
Copy link
Member

dietmarw commented Sep 6, 2022

I think the proposal of @christiankral is the way to go.

Modelica/Blocks/Math.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Math.mo Outdated Show resolved Hide resolved
Modelica/Blocks/package.mo Outdated Show resolved Hide resolved
@beutlich beutlich removed their request for review September 13, 2022 07:59
christiankral and others added 4 commits October 16, 2022 22:55
Co-authored-by: Hans Olsson <[email protected]>
Co-authored-by: Leo Gall <[email protected]>
Co-authored-by: Leo Gall <[email protected]>
@christiankral
Copy link
Contributor

christiankral commented Oct 16, 2022

I think the proposal of @christiankral is the way to go.

@AHaumer @HansOlsson @MartinOtter do you have any objections

@HansOlsson HansOlsson self-requested a review November 11, 2022 10:41
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.

It seems to work.

My only concern is that it may cause unnecessary events in some cases - e.g., using an expsine-signal it seems we will continue to get events every cycle (due to u>=x and u<=x) even though we are far from an extremum.

I don't know exactly how to safely fix that (I can think of several possibilities - the problem is to ensure that they work), so I propose we delay such improvements until needed.

Modelica/Blocks/Math.mo Outdated Show resolved Hide resolved
@beutlich
Copy link
Member

beutlich commented Nov 16, 2022

As already mentioned by @dietmarw in #3762 (comment), it might be worth to point again to the LastLib proof of concept how to calculate the signal extrema without events or derivatives.

Co-authored-by: Thomas Beutlich <[email protected]>
@MartinOtter MartinOtter merged commit 368fe2f into modelica:master Nov 27, 2022
@beutlich beutlich removed the request for review from christiankral February 25, 2023 16:32
@beutlich beutlich added this to the MSL4.1.0 milestone Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants