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

Invalid connection between connectors #1317

Closed
maltelenz opened this issue Apr 18, 2023 · 11 comments
Closed

Invalid connection between connectors #1317

maltelenz opened this issue Apr 18, 2023 · 11 comments

Comments

@maltelenz
Copy link

maltelenz commented Apr 18, 2023

When I validate IDEAS.BoundaryConditions.Examples.SimInfoManager in Wolfram System Modeler, I get (the first of many similar messages):

Error: IDEAS.BoundaryConditions.Interfaces.PartialSimInfoManager [[170:7-170:7]] Invalid connect-equation. sim.radSol[$i_30].solBus and sim.weaBus.solBus[$i_30] do not have compatible causality. Inputs and outputs can only be connected to other inputs and outputs.

which I agree with.

The location referred to is:

connect(radSol[i].solBus, weaBus.solBus[i]) annotation (Line(

In IDEAS.BoundaryConditions.SolarIrradiation.RadSol sim.radSol.solBus is declared as output. I would expect some matching input prefix for the other part of the connection, sim.weaBus.solBus, but was unable to find one.

Did I miss something?

@jelgerjansen
Copy link
Contributor

Thank you for checking this. I get similar errors when I do a pedantic check in Dymola.

The origin of the problem lies in the model declaration of IDEAS.Buildings.Components.Interfaces.SolBus and IDEAS.Buildings.Components.Interfaces.WeaBus. These non-expandable connectors have connectors called RealConnectors which have no input/output specifier, while an expandable connector assigns the causality (input or output) based on the connection that is made.

Therefore, the connection between an input/output with the SolBus or WeaBus throws an error when doing a pedantic check. However, the model still translates and simulates succesfully in both Dymola and OpenModelica.

I'll have to check with the developers of those components why they used this implementation using RealConnectors and I will check whether we can easily solve the issue you raised.

@maltelenz
Copy link
Author

However, the model still translates and simulates succesfully in both Dymola and OpenModelica.

In general, we (Wolfram) try to enforce rules from the language specification in System Modeler, so we treat this as an error in System Modeler.

@Mathadon
Copy link
Member

Mathadon commented Apr 19, 2023

@maltelenz the connector IDEAS.Buildings.Components.Interfaces.WeaBus contains declaration

  IDEAS.Buildings.Components.Interfaces.SolBus[numSolBus] solBus(each outputAngles=outputAngles) annotation ();

This does not have an input/output specifier nor do the connectors defined therein since their causalities depend on which component they are used in. In some cases the variables are inputs and in other cases they are outputs. So it seems inappropriate/counterproductive to assign a causality. I'm not even sure that this is an error since it seems like a completely valid use case. What section of the Modelica specification is this conflicting with?

@maltelenz
Copy link
Author

What section of the Modelica specification is this conflicting with?

https://specification.modelica.org/maint/3.6/connectors-and-connections.html#S3.I1.i5.p1 (emphasis mine):

The matched primitive components of the two connectors must have the same primitive types, and flow variables may only connect to other flow variables, stream variables only to other stream variables, and causal variables (input/output) only to causal variables (input/output).

@Mathadon
Copy link
Member

Mathadon commented Apr 19, 2023

@maltelenz thanks for looking that up for us!

Reading that, I'm wondering whether it's an option to remove the output modifier in IDEAS.BoundaryConditions.SolarIrradiation.RadSol. @maltelenz would that work for you? I.e. then all connected variables have the same type and none of them are input or output.

@maltelenz
Copy link
Author

That doesn't solve the problem, instead I get the same error in other places. Here is the first:

Error: IDEAS.BoundaryConditions.SolarIrradiation.ShadedRadSol [[21:3-21:3]] Invalid connect-equation. sim.radSol[$index_12].relAzi.y and sim.radSol[$index_12].solBus.angAzi do not have compatible causality. Inputs and outputs can only be connected to other inputs and outputs.

Additionally, I guess you would run into the restriction here:
https://specification.modelica.org/maint/3.6/connectors-and-connections.html#balancing-restriction-and-size-of-connectors

For each non-partial non-expandable connector class the number of flow variables shall be equal to the number of variables that are neither parameter, constant, input, output, stream nor flow.

If you want to use input/output, I believe you need to be explicit what is input or output.

@Mathadon
Copy link
Member

Mathadon commented Apr 19, 2023

That error makes sense. There will always be some point where a causal variable must be connected to the bus. So that's not an option. Perhaps it's an option to make an InputWeaBus with

input IDEAS.Buildings.Components.Interfaces.SolBus[numSolBus] solBus(each outputAngles=outputAngles) annotation ();

and an OutputWeaBus with

output IDEAS.Buildings.Components.Interfaces.SolBus[numSolBus] solBus(each outputAngles=outputAngles) annotation ();

and revise all component models that use a weather bus accordingly. Chances are that we'd have to do the same to IDEAS.Buildings.Components.Interfaces.ZoneBus since it also uses RealConnector without causality.

I think this should work for many models but likely not all. One exception is IDEAS.Buildings.Components.Interfaces.DummyConnection since here the causality of the connections through the bus connector changes depending on the value of the parameter isZone. Another exception is IDEAS.Buildings.Components.Interfaces.PartialSurface where there exists a protected propsbus such that connections are only made internally. I think this means we'll have to connect a InputZoneBus to an InputZoneBus, which might not work, or create a second dummy connector. But that would create an even bigger mess with protected connectors than what we have now. Thirdly: there is also a protected connector in IDEAS.Buildings.Components.Interfaces.PartialZone.

My impression is that the Modelica spec is ruling out some valid use cases by constraints quoted by @maltelenz . My suggestion would be to keep the implementation of IDEAS as it is for now, unless someone wants to take on the challenge to fix this of course :)

@Mathadon
Copy link
Member

For each non-partial non-expandable connector class the number of flow variables shall be equal to the number of variables that are neither parameter, constant, input, output, stream nor flow.

God knows, maybe it helps to make the connector expandable.. :p It used to be expandable in the past so it should work. You can give that a try too.

@jelgerjansen
Copy link
Contributor

jelgerjansen commented Apr 20, 2023

Making the connectors expandable (e.g. expandable connector WeaBus) seems to solve the errors, at least in Dymola (pedantic check).
@maltelenz, can you confirm this for Wolfram System Modeler?

Before applying this to all connectors, we first need to set up our CI runner again (#1316) to make sure all our models still work. In the meantime, I'll already create a pull request for this issue.

jelgerjansen added a commit that referenced this issue Apr 20, 2023
…expandable, to avoid errors as mentioned in issue #1317.
@maltelenz
Copy link
Author

I tried running the example using the branch from the pull request in System Modeler, but for some reason System Modeler is taking forever, which would indicate a bug in System Modeler.

I would say that is an inconclusive result with regards to the validity of the model, but if Dymola doesn't complain with pedantic checking, that is probably a good sign.

@Mathadon
Copy link
Member

Mathadon commented Apr 21, 2023

A an expandable connector containing a vector of expandable connectors in an inner/outer component (i.e. the SimInfoManager) is of course a bit complex. And not all tools may fully support this corner case. This is the reason why we removed the expandable in the first place. But that it does not adhere to the spec then adding expandable is probably better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants