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

Wrong flat Modelica output of records with start attributes #11792

Closed
casella opened this issue Jan 8, 2024 · 9 comments
Closed

Wrong flat Modelica output of records with start attributes #11792

casella opened this issue Jan 8, 2024 · 9 comments
Assignees
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend
Milestone

Comments

@casella
Copy link
Contributor

casella commented Jan 8, 2024

Steps to Reproduce

Consider the following MWE:

model M
  record R
    Real x;
    Real y;
  end R;
  
  R r1(x(start = 1), y(start = 2));
  R r2(x(start = 3), y(start = 4));
equation
  r1 = R(0,0);
  r2 = R(0,0);
end M;

When I instantiate it with the default options, I get

function M.R "Automatically generated record constructor for M.R"
  input Real x;
  input Real y;
  output R res;
end M.R;

class M
  Real r1.x(start = 1.0);
  Real r1.y(start = 2.0);
  Real r2.x(start = 3.0);
  Real r2.y(start = 4.0);
equation
  r1 = M.R(0.0, 0.0);
  r2 = M.R(0.0, 0.0);
end M;

However, if I turn on the -f translation flag, I get

model 'M'
record 'R'
  Real 'x'(start = 1.0);
  Real 'y'(start = 2.0);
end 'R';

  'R' 'r1';
  'R' 'r2';
equation
  'r1' = 'M.R'(0.0, 0.0);
  'r2' = 'M.R'(0.0, 0.0);
end 'M';

This is wrong for two reasons:

  1. The start attribute on the two record instances 'r1' and 'r2' were lost in the declarationss
  2. Some of those start attributes are incorrectly assigned to the record type itself.

@perost I hope this is a quick fix

Keeping @mscuttari in the loop

@casella casella added the COMP/OMC/Frontend Issue and pull request related to the frontend label Jan 8, 2024
@casella casella added this to the 1.23.0 milestone Jan 8, 2024
@perost
Copy link
Member

perost commented Jan 16, 2024

#11812 fixes the second issue, that the record declaration itself is wrong. The issue was that the frontend does not have unmodified instances of records, only record instances with modifiers applied (e.g. r1 and r2) and record constructors. With the fix the frontend now reinstantiates each record type we want to dump without modifiers, similar to how we create record constructors.

Regarding the first issue the problem is that the variables in the flat model do not have modifiers, only binding equations and type attributes. But it should be possible to reconstruct the modifiers from the instance tree when dumping record components.

@casella
Copy link
Contributor Author

casella commented Jan 17, 2024

#11812 fixes the second issue, that the record declaration itself is wrong. The issue was that the frontend does not have unmodified instances of records, only record instances with modifiers applied (e.g. r1 and r2) and record constructors. With the fix the frontend now reinstantiates each record type we want to dump without modifiers, similar to how we create record constructors.

Good.

Regarding the first issue the problem is that the variables in the flat model do not have modifiers, only binding equations and type attributes.

I see. So, if I flatten

model M3
  Real x(start = 3);
equation
  der(x) = 1;
end M3;

I get

class M3
  Real x(start = 3.0);
equation
  der(x) = 1.0;
end M3;

but then start = 3.0 is a type attribute of Real?

But it should be possible to reconstruct the modifiers from the instance tree when dumping record components.

I guess that is necessary, if we want to support records in BaseModelica output

@perost
Copy link
Member

perost commented Jan 17, 2024

I see. So, if I flatten

model M3
  Real x(start = 3);
equation
  der(x) = 1;
end M3;

I get

class M3
  Real x(start = 3.0);
equation
  der(x) = 1.0;
end M3;

but then start = 3.0 is a type attribute of Real?

Yes, each variable has a flat list of type attributes (just a list of name/binding tuples). There's never been any need to be able to represent nested binding equations before since those are normally resolved during flattening.

@casella
Copy link
Contributor Author

casella commented May 21, 2024

OK, now after #11812 I get

package 'M'
  record 'R'
    Real 'x';
    Real 'y';
  end 'R';

  model 'M'
    'R' 'r1';
    'R' 'r2';
  equation
    'r1' = 'M.R'(0.0, 0.0);
    'r2' = 'M.R'(0.0, 0.0);
  end 'M';
end 'M';

@perost, getting the record member modifiers is essential if we want to get power grids models to run in MARCO. Do you think you can fix this before the summer break? Thanks!

@perost
Copy link
Member

perost commented May 22, 2024

#12453 improves the handling of records in Base Modelica, the output is now:

package 'M'
  record 'R'
    Real 'x';
    Real 'y';
  end 'R';

  model 'M'
    'R' 'r1'('x'(start = 1.0), 'y'(start = 2.0));
    'R' 'r2'('x'(start = 3.0), 'y'(start = 4.0));
  equation
    'r1' = 'M.R'(0.0, 0.0);
    'r2' = 'M.R'(0.0, 0.0);
  end 'M';
end 'M';

There's still an issue here, M.R in the equation is wrong since the record is just called R, but that's probably not hard to fix.

However, I'm not sure if we actually want this fix, because it changes the flattening to do the same as for the new backend and not split record instances instead of having to reconstruct them when dumping Base Modelica. I realized after implementing the fix that the complicated and error-prone way we previously did it was to try to avoid having the --baseModelica flag influence the flat model that we pass to the backend.

With this fix that's no longer the case, since using --baseModelica will now keep record instances in the flat model which for example the old backend can't handle, so dumping Base Modelica and also compiling the model in the same pass doesn't work. That could be a big issue if we have any plans on making Base Modelica the default output.

@perost
Copy link
Member

perost commented May 22, 2024

#12453 has now been replaced by #12454, which achieves the same result but without changing the flattening behaviour. It turned out that the current code only needed some very slight modification to work with the new code I wrote to dump record modifiers. The reconstruction of record instances still has issues, like probably not working with nested records, but that's a different issue.

@perost
Copy link
Member

perost commented May 22, 2024

#12455 fixes the issue I mentioned earlier that record expressions use the wrong name for the record type.

@casella
Copy link
Contributor Author

casella commented May 22, 2024

The reconstruction of record instances still has issues, like probably not working with nested records, but that's a different issue.

That is not really necessary for the time being. We just need to be able to handle Complex numbers.

@casella casella modified the milestones: 1.23.0, 1.24.0 Jun 11, 2024
@casella
Copy link
Contributor Author

casella commented Jun 17, 2024

I checked with the latest nightly, All fine, thanks @perost!

@casella casella closed this as completed Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend
Projects
None yet
Development

No branches or pull requests

2 participants