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

Multiple modifications and merging of modifications. #3474

Open
qlambert-pro opened this issue Feb 5, 2024 · 18 comments · May be fixed by #3545
Open

Multiple modifications and merging of modifications. #3474

qlambert-pro opened this issue Feb 5, 2024 · 18 comments · May be fixed by #3545
Assignees
Labels
clarification Specification of feature is unclear, but not incorrect
Milestone

Comments

@qlambert-pro
Copy link
Collaborator

qlambert-pro commented Feb 5, 2024

I think there are some corner cases in the handling of modifications where we need some explicit clarifications.
#3339 contains some examples.
Here are a few more models:

model Test
  model A
    replaceable Real x = 1;
  end A;

  A a(redeclare Real x, x.start = 2);
end Test;
model Test2
  model A
    replaceable Real x = 1;
  end A;

  model A1
    extends A(x.start = 2);
  end A1;

  A1 a(redeclare Real x);
end Test2;
model Test3
  model A
    replaceable Real x = 1;
  end A;

  model A1
    extends A(x.start = 1);
  end A1;

  type R = Real(start = 2);
  A1 a(redeclare R x);
end Test3;

I would consider all of them valid, and after resolving the modifications, I would expect:
Test.a.x.start = 2
Test2.a.x.start = 2
Test3.a.x.start = 2

What do other people think?

@qlambert-pro qlambert-pro added the clarification Specification of feature is unclear, but not incorrect label Feb 5, 2024
@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Feb 6, 2024

Here are other, weird cases:

model Test4
  model B
    replaceable Real m = 3;
  end B;

  model A
    extends B(replaceable Real m = 1);
  end A;

  extends A(redeclare Real m);
end Test4;
model Test5
  model C0
    Real x;
  end C0;

  model C1
    extends C0;
    Real y = 1;
  end C1;

  model B
    replaceable C1 c constrainedby C0;
  end B;

  model A
    extends B(replaceable C1 c(y = 2));
  end A;

  extends A(redeclare C1 c);
end Test5;

@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Feb 7, 2024

Should there be a difference between the following models:

model Test6
  model A
    replaceable Real x = 4;
  end A;

  model B
    A a(replaceable Real x = 1);
  end B;

  B b(a(redeclare Real x));
end Test6;
model Test7
  model A
    replaceable Real x = 4;
  end A;

  model B
    A a(replaceable Real x = 1 constrainedby Real);
  end B;

  B b(a(redeclare Real x));
end Test7;
model Test8
  model A
    replaceable Real x = 4;
  end A;

  model B
    A a(x = 1);
  end B;

  B b(a(redeclare Real x));
end Test8;

@HansOlsson HansOlsson added this to the 2024-1 milestone Feb 15, 2024
@HansOlsson
Copy link
Collaborator

HansOlsson commented Feb 22, 2024

Great, I managed to write a detailed reply and then close the window before commenting and then I lost power...

Here the tests go again in Dymola:

  • Test - error (multiple modification); I'm not 100% on this one - but I think it should be an error
  • Test2.x.start=2
  • Test3.a.x.start=1 (component modifiers take precedence over class modifiers)
  • Test4.a.x=1 should be 3 (see below)
  • Test5.c.y=2 should be 1 (see below)
  • Test6.b.a.x=1 should be 4 (see below)
  • Test7.b.a.x=4
  • Test8.b.a.1=1 (normal modifiers are applied to both redeclare and constraining type)

The issue for Test4, Test5, and Test6 is that Dymola for some reason doesn't handle the implicit "redeclare" for these replaceable components (so if you use redeclare replaceable it gives the desired result) . In addition the part about keeping constrainedby in replaceable declaration isn't super-clear for components, but if we consider this variant of Test5 it becomes clear:

model Test5A
  model C0
    Real x;
  end C0;

  model C1
    extends C0;
    Real y = 1;
  end C1;

  model C2
    extends C0;
    Real z = 1;
  end C2;

  model B
    replaceable C1 c constrainedby C0;
  end B;

  model A
    extends B(/*redeclare*/ replaceable C1 c(y = 2));
  end A;

  extends A(redeclare C2 c);
end Test5A;

Clearly it doesn't make sense to keep the modifier and use it with an incompatible class.

@gkurzbach
Copy link
Collaborator

SimulationX;
Test1: a.x.start=2 (it converts the double modification to A a(redeclare Real x(start=2));.
Test2: a.x.start=2
Test3: a.x.start=1
Test.4: m=3
Test5: c.y=1
Test6: b.a.x=4
Test7: b.a.x=4
Test8 b.a.x=1
Test 5A: c.z=1

@HansOlsson
Copy link
Collaborator

I believe I have found a good reason why we shouldn't handle the double modification in Test(1).
(After ensuring that the next version of Dymola fully handles the other cases.)

Consider the following variants of Test5A:

model Test5B
  model C0
    parameter Real x;
  end C0;

  model C1
    extends C0;
    Real y = 1;
  end C1;

  model C2
    extends C0;
    Real z = 1;
  end C2;

  model B
    replaceable C1 c constrainedby C0;
  end B;

  model A1
    extends B(c.x = 2);
  end A1;
  
  model A2
      extends A1(redeclare replaceable C1 c(y=2));
  end A2;

  extends A2(redeclare C2 c);
end Test5B;

That model gives c.x=2, (swapping A1 and A2 wouldn't influence this result).

If we want to merge A1 and A2 into one class that seems to have modifier redeclare replaceable C1 c(y = 2), c.x=2 and we might think it suffices to write:

  model A2
    extends B(redeclare replaceable C1 c(x = 2, y=2));
  end A2;

but then Test5B.c.x no longer has a value (since the value is only part of the redeclaration and not the constraint).
(I added the y=2 just to show why merging any modifier can be problematic.)

Since merging a value-modifier with redeclare replaceable modifier subtly influences the result, and I wouldn't want that to happen automatically. It doesn't happen with a pure redeclare (since we cannot redeclare it again), but I would still find it confusing if it worked in some cases.

A working way to combine the redeclaration and the modifier is:

  model A2
    extends B(redeclare replaceable C1 c(y=2) constrainedby C0(x = 2));
  end A2;

It is a bit esoteric (since the use-case is esoteric), but it clearly indicates the differences.

The only down-side is that it is necessary to repeat the constrainedby-class.
I guess we could support dropping the class leading to: redeclare replaceable C1 c(y=2) constrainedby (x = 2); as it should still be unambiguous to parse - but it doesn't seem that common, is a bit hard to understand; and may cause more confusion. I don't think we should support redeclare replaceable C1 c(y=2), c(x = 2) as an alias for that (it seems too cute).

@qlambert-pro
Copy link
Collaborator Author

Given how modifications work the redeclare replaceable C1 c(y=2) constrainedby (x = 2) interpretation seems like the logical one to me.

@HansOlsson
Copy link
Collaborator

Given how modifications work the redeclare replaceable C1 c(y=2) constrainedby (x = 2) interpretation seems like the logical one to me.

Does that mean that there's agreement on all issues? In that case the next question is whether we need to change something in the specification.

@eshmoylova
Copy link
Member

I agree on the results. The first version I tried for Test4, Test5, Test6 gave the same result as in Dymola. But I found that there was a bug that we were merging modifications from an outer redeclaration with an inner redeclaration. I am not sure that the point that modifications from an intermediate redeclaration is dropped with a subsequent redeclaration is made clear in the specification. The only reference that I see to it is in the comment about T0 in the example of Circuit3. And it took some purposeful searching to find that.

As for the double modifications in Test(1), if redeclare replaceable C1 c(y = 2), c.x=2 is not equivalent to redeclare replaceable C1 c(y = 2, x=2), it needs to be specified. We treat them as equivalent. From @gkurzbach's result, it looks like they also consider them the same.

@HansOlsson
Copy link
Collaborator

I agree on the results. The first version I tried for Test4, Test5, Test6 gave the same result as in Dymola. But I found that there was a bug that we were merging modifications from an outer redeclaration with an inner redeclaration. I am not sure that the point that modifications from an intermediate redeclaration is dropped with a subsequent redeclaration is made clear in the specification. The only reference that I see to it is in the comment about T0 in the example of Circuit3. And it took some purposeful searching to find that.

As for the double modifications in Test(1), if redeclare replaceable C1 c(y = 2), c.x=2 is not equivalent to redeclare replaceable C1 c(y = 2, x=2), it needs to be specified. We treat them as equivalent. From @gkurzbach's result, it looks like they also consider them the same.

As indicated above I believe we need to specify that due to the different between default for the replaceable component, and default for its constraining type. We should likely have an example as well for this.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Jun 17, 2024

Looking through the options here.
I hope we all agree that (redeclare replaceable C1 c(y = 2, x=2)) and (redeclare replaceable C1 c(y = 2) constrainedby C0(x=2)) (where C0 is the original constraint) are subtly different.

To me it makes the most sense that (redeclare replaceable C1 c(y = 2), c.x=2) is the same as (redeclare replaceable C1 c(y = 2) constrainedby C0(x=2)).

Note that (c.x=2) and ((c(x=2)) are still allowed.

However, I agree that it is awkward and would propose:

  1. The following change in the grammar:
constraining-clause :
   constrainedby ( type-specifier [ class-modification ] | class-modification )

where the part after "|" is new. This allows directly writing redeclare replaceable C1 c(y = 2) constrainedby (x=2).
2. The constraint class for a constrainedby without type-specifier is the default which for a modifier is the previous constraint and for a declaration the used class.
3. Use this to define handling of multiple modifications for replaceable modifiers (the other cases are simpler).

Some design considerations:

  • Do not Repeat Yourself; the constraining class is no longer needed if we only want to modify the constraint-modifiers without changing the constraining class.
  • The class-modification is required if there is no type-specifier, as it is otherwise hard to parse for humans (and possibly for computers).
  • It is allowed with (roughly) the same meaning for both modifiers and declarations.
  • An empty parenthesis is allowed here, but constrainedby () can be removed without changing anything for a replaceable declaration/modifier. (I was going to write "not recommended", but then I realized that some might like it for consistency reasons etc, so I skipped that.)
  • This allows:
    • A mapping of multiple modifications to normal modifications (as indicated above).
    • A local "merging" of modifications in multiple levels.

I tried to look at constrainedby in MSL and only found the following case that could be simplified:

  • Modelica.Clocked.ClockSignals.Clocks.Logical.PartialLogicalClock

The following cases could just drop the constrainedby:

  • Modelica.Fluid.Machines.BaseClasses.PartialPump.Monitoring
  • Modelica.Electrical.Machines.Examples.ControlledDCDrives.Utilities.PartialControlledDCPM

@eshmoylova eshmoylova modified the milestones: 2024-2, 2024-June Jun 24, 2024
@HansOlsson
Copy link
Collaborator

Phone meeting: An alternative is to interpret it as (redeclare replaceable C1 c(y = 2, x=2)) and in the specification explain why this differs.

@HansOlsson
Copy link
Collaborator

Phone meeting: An alternative is to interpret it as (redeclare replaceable C1 c(y = 2, x=2)) and in the specification explain why this differs.

Hmm.. Thinking more I realized that there are two other cases:

If we have: (redeclare replaceable C1 c constrainedby C0(y=2), c.x=1) or (redeclare replaceable C1 c(z=3) constrainedby C0(y=2), c.x=1) it would be odd to merge it to: (redeclare replaceable C1 c(x=1) constrainedby C0(y=2)) or (redeclare replaceable C1 c(x=1, z=3) constrainedby C0(y=2)) - and having it on the constrainedby makes more sense.

To me this indicates that we should just report it as an error at the moment, and make that clear in the specification.

  1. If we want to extend the semantics, I would say we should also have a general handling of "redeclare" where both the constraining type and new type can be left out - so that we can write something like: redeclare replaceable c(x=1) constrainedby (y=2). or just replaceable c(x=1). This additionally solves the issue of modifying the default-parameters without having to repeat the default type.

@HansOlsson HansOlsson linked a pull request Jun 27, 2024 that will close this issue
@qlambert-pro
Copy link
Collaborator Author

  partial model Base
    parameter Real p;
  end Base;

  model Implementation
    extends Base;
    parameter Real q;
  end Implementation;

  model A
    replaceable Base b constrainedby Base(p = 1);
  end A;

  model B
    A a(redeclare replaceable Implementation b constrainedby Base, b.q = 1);
    // Is that modifier supposed to be moved into b() or into Base()?
  end B;

  model C
    B b(a(redeclare replaceable Base b));
  end C;
end Test;

WSM finds Test.B to be valid and Test.C to be invalid. We interpret it as a modifier to Base() in the same way as it would be if there was no redeclare modification standing next to it.

@HansOlsson
Copy link
Collaborator

  partial model Base
    parameter Real p;
  end Base;

  model Implementation
    extends Base;
    parameter Real q;
  end Implementation;

  model A
    replaceable Base b constrainedby Base(p = 1);
  end A;

  model B
    A a(redeclare replaceable Implementation b constrainedby Base, b.q = 1);
    // Is that modifier supposed to be moved into b() or into Base()?
  end B;

  model C
    B b(a(redeclare replaceable Base b));
  end C;
end Test;

WSM finds Test.B to be valid and Test.C to be invalid. We interpret it as a modifier to Base() in the same way as it would be if there was no redeclare modification standing next to it.

Dymola doesn't do that, and thus I think it is best to not define that as different tools have different opinions and it seems better that users clarify their intent.

BTW: Do I understand it correctly that you interpret A a(redeclare replaceable Implementation b constrainedby Base, b.q = 1); as A a(redeclare replaceable Implementation b constrainedby Base(q = 1));, but A a(redeclare replaceable Implementation b, b.q = 1); as A a(redeclare replaceable Implementation b(q=1)); even though the constrainedby is sort of redundant?

@qlambert-pro
Copy link
Collaborator Author

I misremembered during the meeting, both cases are handled the same, as you see it A a(redeclare replaceable Implementation b constrainedby Base(q = 1));. The modification is not inside a redeclare so it is not handled as if it was. It is handled in exactly the same as if the redeclare wasn't there.

My way of seeing it is that we handle the modifications in the same way we would if they came from different scopes, except for the usual case where q would be modified twice (which would indeed be ambiguous in a single list of modifications).

Dymola doesn't do that, and thus I think it is best to not define that as different tools have different opinions and it seems better that users clarify their intent.

If there is a strong consensus among the different tools, I would much rather go with that interpretation than forbidding it because it wasn't unanimous.

@HansOlsson
Copy link
Collaborator

I misremembered during the meeting, both cases are handled the same, as you see it A a(redeclare replaceable Implementation b constrainedby Base(q = 1));. The modification is not inside a redeclare so it is not handled as if it was. It is handled in exactly the same as if the redeclare wasn't there.

But how does that work if you start from A a(redeclare replaceable Implementation b, b.q=1?

Is it just internally handled in that way - as the Base-class isn't known, or is there some additional trick?

@qlambert-pro
Copy link
Collaborator Author

qlambert-pro commented Jun 28, 2024

I don't understand the question. I am not sure there are additional tricks, but I did find the whole thing, extremely tricky to implement.
We process
A a(redeclare replaceable Implementation b(p = 2)), b.q=1) in the same way you would process A a(b(p = 2), b.q=1), we just remember that b.p = 2 came from a replaceable redeclaration of b.

@HansOlsson
Copy link
Collaborator

HansOlsson commented Jun 28, 2024

I don't understand the question. I am not sure there are additional tricks, but I did find the whole thing, extremely tricky to implement. We process A a(redeclare replaceable Implementation b(p = 2)), b.q=1) in the same way you would process A a(b(p = 2), b.q=1), we just remember that b.p = 2 came from a replaceable redeclaration of b.

Ok, I see.

The problem is then that the current specification describes the handling of such modifiers as a form of syntactic sugar for the merged variant, and in this case it is impossible if there is no constrainedby. Obviously we don't have to describe it as syntactic sugar in all cases (or even at all), so I guess the suggestion would instead be to just describe how the nested modifiers are handled.

See proposal in #3545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Specification of feature is unclear, but not incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants