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 discrete intermediate variable to Mean block to clarify that the block output is a continuous-time variable #3269

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Dec 17, 2019

Note, that Mean.y is marked as discrete output.

Not sure if these blocks rather belong to Modelica.Blocks.Discrete.

@beutlich beutlich added enhancement New feature or enhancement L: Blocks Issue addresses Modelica.Blocks labels Dec 17, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Dec 17, 2019
@beutlich beutlich self-assigned this Dec 17, 2019
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.

I don't understand the reason for these changes.

Having "discrete" on an output is pretty useless in Modelica; as it doesn't propagate to the input (when connecting the block to another block).
Not using the base-class "SISO" causes code duplication (especially since there is no DiscreteSISO) - and there are some more complicated consequences of that in Dymola.

@beutlich
Copy link
Member Author

beutlich commented Dec 17, 2019

Well, I started with Mean, which is a discrete block by design, when using the inheritance from DiscreteBlock. Additionally, SystemModeler complained about output y not being made discrete, which is the reason why I did not extend from DiscreteSISO (in Interfaces), but from DiscreteBlock.

Then, I noticed that RectifiedMean, RootMeanSquare, Harmonic all use the Mean block as component by passing the sampling frequency. For that reason I changed the icon from SISO to Discrete for these three blocks, too.

@HansOlsson
Copy link
Contributor

Well, I started with Mean, which is a discrete block by design, when using the inheritance from DiscreteBlock. Additionally, SystemModeler complained about output y not being made discrete, which is the reason why I did not extend from DiscreteSISO (in Interfaces), but from DiscreteBlock.

The reason y cannot be discrete in DiscreteSISO is that the "discrete" definition in Modelica isn't propagated and thus Mean may have a discrete output but not RectifiedMean (as currently implemented; Clocked signals are better in that way).

And you cannot detect it when connecting to the output so, I don't see why making the output "discrete" is important. I understand that there has been some issues related to that in SystemModeler - but that is how Modelica is defined. On the other hand not using inheritance will cause problems in Dymola (even though it should work the same way in Modelica).

However, the base Interfaces.DiscreteBlock is also problematic, and complicates the code for little gain (you have to switch between classes to understand the new class, including realizing that the extra protected outputs aren't used etc).

In general my view is that if we duplicate code instead of using Interfaces-classes there is some problem with the design of the Interface-classes; and we should first fix that.

@beutlich
Copy link
Member Author

OK, I see. Let's consider the two issues here. Mean and the 3 other blocks.

  1. Looking at Mean I believe it is cleaner now. I dropped the discrete output y again.
  2. Yes, some new base class helps to get things grouped together. Unfortunately, DiscreteSISO already is taken.

@MartinOtter
Copy link
Member

An easy way to fix this in a clean way is:

block Mean "Calculate mean over period 1/f"
  extends Modelica.Blocks.Interfaces.SISO;
  parameter Modelica.SIunits.Frequency f(start=50) "Base frequency";
  parameter Real x0=0 "Start value of integrator state";
  parameter Boolean yGreaterOrEqualZero=false
    "=true, if output y is guaranteed to be >= 0 for the exact solution"
    annotation (Evaluate=true, Dialog(tab="Advanced"));
protected 
  parameter Modelica.SIunits.Time t0(fixed=false) "Start time of simulation";
  Real x "Integrator state";
  Real y_last;
initial equation 
  t0 = time;
  x = x0;
  y_last = 0;
equation 
  der(x) = u;
  when sample(t0 + 1/f, 1/f) then
    y_last = if not yGreaterOrEqualZero then f*pre(x) else max(0.0, f*pre(x));
    reinit(x, 0);
  end when;
  y = y_last
end Mean;

The output is then clearly a continuous-time variable.

@beutlich
Copy link
Member Author

Probably with discrete Real y_last;, right?

@MartinOtter
Copy link
Member

Probably with discrete Real y_last;, right?

Yes

@beutlich beutlich changed the title Mark Modelica.Blocks.Math.{Mean, RectifiedMean, RootMeanSquare, Harmonic} as discrete blocks Add discrete intermediate variable to Mean block Dec 17, 2019
@beutlich
Copy link
Member Author

Now SystemModeler no longer complains. Looks better and cleaner now indeed. Thanks.

@beutlich beutlich changed the title Add discrete intermediate variable to Mean block Add discrete intermediate variable to Mean block to clarify that the block output is a continuous-time variable Dec 17, 2019
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.

Ok. That seems like a much simpler solution.
Just a tiny change: Change the initial equation to "y_last=0" instead of "y=0".

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.

Ok. I still don't see why it is necessary, but at least it doesn't break anything.

@beutlich
Copy link
Member Author

beutlich commented Dec 18, 2019

Two reasons for the change:

  1. SystemModeler raises a warning that a variable is not declared discrete. (We could argue: "tool-issue" - so what or try to fix it in a common sense.)
  2. Like I initially did, the previous implementation of Mean could be rewritten to extend from DiscreteBlock or DiscreteSISO, which somehow implies theat Mean is a discrete block, i.e., a block with a discrete output variable. Now, with the intermediate discrete variable y_last, it is clarified that output y is a continuous-time variable.

@beutlich beutlich merged commit 2e42302 into modelica:master Dec 18, 2019
@beutlich beutlich deleted the mark-as-discrete branch December 18, 2019 19:41
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.

None yet

3 participants