-
Notifications
You must be signed in to change notification settings - Fork 297
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
NB unmatched variables and equations in PowerGrids #11556
Comments
It seems like Or maybe this is still correct. It is not a top-level input, so it is unknown right? |
The flat Modelica output of that model has this declaration Real infiniteBus.port.v.re(quantity = "ElectricPotential", unit = "V", displayUnit = "kV", start = infiniteBus.port.vStart.re, nominal = infiniteBus.port.VBase) "Real part of complex number"; I can't see any |
For the old backend the frontend modifies the variable direction here when generating the DAE structure. Potentially we should move that to an earlier phase so it applies to both backends. |
I agree. The issue in PowerGrids.Electrical.Test.OneBusOneLoad is probably stemming from the I guess simply removing the input prefix for non-top-level inputs is enough to handle these cases. @perost, can you please go ahead with that? This may be the last pending issue before we can actually run PowerGrids with the NB. In fact, as soon as we can do that, we should also solve #10034, otherwise the dense solver won't be able to handle models with more than a few dozen buses, but that's a runtime issue. |
@perost if you can try it today and we are lucky, I could actually mention running PowerGrids in the report for RTE, which would be great. There's time until tomorrow to do that 😅 . You can use the model mentioned at the beginning of the ticket as a test case. |
Implemented in #11562, but it seems we're not lucky because it doesn't seem to change much. The NB still thinks that I also tried removing the direction from the children of the Variable:s, but it also made no difference. I didn't include that in my PR since I'm not sure if it matters or not, maybe @kabdelhak has some opinion on that. But regardless it seems this is now an issue with the NB that we need to look into rather than a frontend issue. |
@kabdelhak please check the following MWE: package NonTopLevelInputs
model M1
input Real e;
end M1;
model M2
parameter Real p = 1;
extends M1(e = p);
end M2;
model S
M2 m2;
annotation(
__OpenModelica_commandLineOptions = "--newBackend");
end S;
model M1c
input Complex e;
end M1c;
model M2c
parameter Complex p = Complex(1);
extends M1c(e = p);
end M2c;
model Sc
M2c m2;
annotation(
__OpenModelica_commandLineOptions = "--newBackend");
end Sc;
model M1cv
Complex e;
end M1cv;
model M2cv
parameter Complex p = Complex(1);
extends M1cv(e = p);
end M2cv;
model Scv
M2cv m2;
annotation(
__OpenModelica_commandLineOptions = "--newBackend");
end Scv;
end NonTopLevelInputs; Model Model
However, also model
So, I guess the problem is not the non-top-level inputs, but rather the [ALGB] variables not being matched correctly with the [RECD] variables. If I flatten Scv with class NonTopLevelInputs.Scv
parameter Complex m2.p = Complex(1.0, 0.0);
Complex m2.e = m2.p;
end NonTopLevelInputs.Scv; so there is probably some pending issue with how the NB handles record equations. For Complex numbers, it is always convenient to split such equations in their real and imaginary counterparts. @kabdelhak can you please have a look? |
- make correct adjacency matrix entries for records - split record equations to their attribute equations once converted to statements (code gen cant handle records on the rhs apparently) - add testsuite example - fixes minimal example of OpenModelica#11556
* [NB] update record handling - make correct adjacency matrix entries for records - split record equations to their attribute equations once converted to statements (code gen cant handle records on the rhs apparently) - add testsuite example - fixes minimal example of #11556 * [testuite] update test file
The PowerGrids.Electrical.Test.OneBusOneLoad model now fails with
Initially we had 8 unmatched scalar variables and 6 unmatched scalar equations, now we are left with just two unmatched scalar variables, so there were actually two separate issues in this ticket. #11585 fixed the first one, I hope the other one is the last before victory 😄 |
Please also check PowerGrids.Examples.ENTSOE.PowerFlow, which gives:
|
The original problem is fixed with #11827. I did not verify the results yet, but it runs. Despite that it will still produce following message due to consistency checks not being implemented yet:
|
Great news! 😃
Yeah, I should generate reference results with the old backend and upload them somewhere. Not enough time to do it.
That's weird, these models should not have any overdetermined initialization, nor high index. In fact, if you check the report with the OB, there is no warning of redundant initial equations needing to be removed. I guess there is still some equation counting issue in the initial equations or something. |
BTW, Adrian fixed the regression reports, so now we have the one regarding the newInst-newBackend job of the 16th: https://libraries.openmodelica.org/branches/history/newInst-newBackend/2024-01-07%2020:12:41..2024-01-16%2017:34:57.html The two improvements were: I'll start another newInst-newBackend job right away, so we check the effects of your latest commit. |
Here is the last regression report. Some progress, but nothing dramatic. |
The PR didn't go through, i pushed the last changes and it should be merged to main in an hour. We can do another library test to check. But we can also wait for more changes, planning on several small fixes this week. |
My guess is that i might be creating more initial equations than necessary. Either due to record bindings which might be created for the records themselves and their elements which leads to basically duplicated equations, or maybe i don't realize some parameters have Either way, i will have a look. |
* [NB] update alias module to not use record elements [NSimCode] draft: inline record residuals * [NB] add variable record children when adding records * [NB] inline record residual equations - fixes #11556 * [testsuite] udapte tests - alias module does not apply to record elements anymore
Everything is merged, my PR automatically closed this issue, feel free to reopen if its still relevant. Using
We can see that there are multiple binding equations for the same thing. Record elements:
And full record:
Which seems like they could be the same equations. I will look and see why there are duplicate bindings, probably expanding the records does that. |
I just started a newInst-newBackend job to test it on the whole library, as soon as the job is finished, the PowerGrids report will be here. |
@kabdelhak, I also created #11852 for the duplication issue, so we keep track of it. |
Description
Please check PowerGrids.Electrical.Test.OneBusOneLoad. This is the simplest phasor-based model of the library. It has 83 variables and equations.
The NB currently fails during the matching phase with:
@kabdelhak can you please try to figure out why the matching fails? Once this model works, we may have ScalableTestGrids running with the NB.
The text was updated successfully, but these errors were encountered: