OpenModelica / OpenModelica

OpenModelica is an open-source Modelica-based modeling and simulation environment intended for industrial and academic usage.
https://openmodelica.org
Other
846 stars 306 forks source link

Non-partial models which are not balanced should generate an error in the NF when checking and trying to simulate #8024

Open casella opened 3 years ago

casella commented 3 years ago

Consider the following MWE

model M
  Real x, y;
equation
  x = 3;
end M;

If I check the model, I get

[1] 11:31:10 Scripting Notification
Check of M completed successfully.

In fact, this model is not partial, so it should be balanced. If it isn't, that is for sure an error. This is currently reported by the backend later on when trying to simulate

[1] 11:32:11 Symbolic Error
Too few equations, under-determined system. The model has 1 equation(s) and 2 variable(s).

but in fact, this information is already available to the NF, which should report it.

Proposed output when checking:

[1] 11:31:10   Symbolic Error
The model M is not balanced.
perost commented 3 years ago

The NF does not do such checks, so what you're proposing is that we always run CheckModel after the frontend. Or I guess we might want to only do it if we're not going to run the backend afterwards, since that's one of the first things the backend then does.

It would probably be more useful to implement the checks for locally balanced models instead, since that's something only the frontend can really do. But that's a lot more work.

perost commented 3 years ago

Sorry, I missed that you were already doing check model. In that case you get the message:

Check of M completed successfully.
Class M has 1 equation(s) and 2 variable(s).
1 of these are trivial equation(s).

OMEdit shows this in a popup window though, and for some reason it then displays only the first line in the message browser. So what you're asking for is already given, but perhaps the presentation could be improved. But that's an OMEdit issue in that case.

casella commented 3 years ago

Or I guess we might want to only do it if we're not going to run the backend afterwards, since that's one of the first things the backend then does.

That was my idea. So is CheckModel part of backend?

Anyway, I'm not talking about deep structural analysis, just checking that the number of equations matches the number of unknowns globally. We already compute those two numbers when doing CheckModel, so it seems trivial to me. Only, we should avoid that for partial models.

It would probably be more useful to implement the checks for locally balanced models instead, since that's something only the frontend can really do.

Of course, we should do that sooner or later, see #3977.

But that's a lot more work.

Of course.

casella commented 3 years ago

Sorry, I missed that you were already doing check model. In that case you get the message:

Check of M completed successfully.
Class M has 1 equation(s) and 2 variable(s).
1 of these are trivial equation(s).

My point is that, since the model is not partial, in this case the outcome should not be "Check of M completed successfully" but rather "Model M is not balanced". Which is an error, not a notification.

perost commented 3 years ago

That was my idea. So is CheckModel part of backend?

No, CheckModel is the module used by the API, i.e. what OMEdit uses when you press the Check Model button. The backend has its own counting, and the frontend doesn't count at all.

The "Check of M completed successfully"-message is given because the check itself did complete successfully regardless of if the model is balanced or not. We might want to change the message logged in the message browser though, but that's something for @adeas31 to look into in that case.

Maybe we should improve the API so that OMEdit can get the number of variables and equations directly, instead of just getting a pre-formatted string (I guess OMEdit could parse the string, but that's not very robust).

adeas31 commented 3 years ago

8033 removes the extra message added to Messages Browser. Now the issue is related to interactive environment. checkModel api returns the result or empty string. In case of empty string the error is shown in the Messages Browser otherwise the returned string is shown in pop up window.

adeas31 commented 3 years ago

The simplest fix is to remove the message "Check of M completed successfully" from the output of checkModel and simply show the rest of the result i.e.,

class M has 1 equation(s) and 2 variable(s).
1 of these are trivial equation(s).
casella commented 3 years ago

That was my idea. So is CheckModel part of backend?

No, CheckModel is the module used by the API, i.e. what OMEdit uses when you press the Check Model button. The backend has its own counting, and the frontend doesn't count at all.

OK. I assume it first calls the NF and then does some extra stuff, e.g. counting, right?

The "Check of M completed successfully"-message is given because the check itself did complete successfully

I understand that, but it is wrong. We need to fix the CheckModel module so it does not report successful completion if the two numbers (variables and equations) are not the same.

We might want to change the message logged in the message browser though, but that's something for @adeas31 to look into in that case.

We need to change both: the one in the message browser, and the one in the checkModel window, wherever they come from.

Maybe we should improve the API so that OMEdit can get the number of variables and equations directly, instead of just getting a pre-formatted string (I guess OMEdit could parse the string, but that's not very robust).

Nah, don't bother, just return the right pre-formatted string, which in this case is "Error: Model is not balanced"

casella commented 3 years ago

The simplest fix is to remove the message "Check of M completed successfully" from the output of checkModel and simply show the rest of the result i.e.,

class M has 1 equation(s) and 2 variable(s).
1 of these are trivial equation(s).

This can work, but I think it is much better if we provide the right output of checkModel 😃

Once again, if the model is not partial and not balanced, it is an error, because there is no way it can be simulated. Why should checkModel say that all is fine?

casella commented 3 years ago

Wait, the thing is slightly more complicated.

If you check a Modelica.Fluid model with unredeclared Medium package, it may lack some equations in the BaseProperties models (which are partial). Of course that doesn't mean the model is bad, it's just that is still contains some partial stuff, so it cannot be simulated.

So, checkModel should report an error if the number of equations does not match the number of variables, but only in case the model, or any of its components is partial.

@perost, I understand that during checkModel, the NF is called with some flag that disables error generation when instantiating partial classes. In that case, we should not generate an error, but we should also report to the caller that there is some partial stuff in the model, so that also checkModel does not complain about the model not being balanced. Does the called function already return this information?

perost commented 3 years ago

@perost, I understand that during checkModel, the NF is called with some flag that disables error generation when instantiating partial classes. In that case, we should not generate an error, but we should also report to the caller that there is some partial stuff in the model, so that also checkModel does not complain about the model not being balanced. Does the called function already return this information?

No, CheckModel works by just calling the frontend and then counting the number of variables and equations in the returned DAE. In other words, CheckModel currently only counts the number of variables and equations, it makes no judgement on whether the model can actually be simulated or not.

casella commented 3 years ago

Well, how deep CheckModel goes with the structural analysis is ultimately up to us to decide.

Going too deep is not a good idea, because if the component is unconnected, and you apply default connection equations (i.e. all flow variables = 0) they can make the model singular, e.g., OnePort electrical models will have two equations saying that the current is zero. That doesn't mean Resistor model (which is balanced BTW) is bad, only that you cannot simulate it without connecting it to appropriate boundary conditions, e.g. two voltage sources or one voltage source and one current source (not two current sources, though).

But at least we can say something about overall balancedness. As I understand it, a model in Modelica 3.x is always supposed to be balanced, unless is partial, or some of its components are partial, in which case the model is still fine, except that you need to extend from it, or to redeclare its partial stuff, in order to actually be able to use it in a simulation model. We could report that when checking the model by itself, but it would just be a notification, not an error, there's nothing bad a partial model within a library, nor in a model that uses a partial Medium package, we have plenty.

So, if CheckModel had this information

it could say something more about the model being good or not. Not necessarily about the fact that the model can be simulated, that was not correct on my side.

The first piece of information is there, so that's easy, just check if the two numbers are the same. The question is, can the NF return the second and third to CheckModel? In that case, we can add a last check to CheckModel, of course assuming all previous ones were successful:

if model_is_partial then 
  message = "Check of" + modelName+" completed successfully. The model is partial, so it need to be extended to be used.";
elseif model_contains_partial_stuff then
  message = "Check of" + modelName+" completed successfully. Some classes in it need to be redeclared to use it.";
elseif n_var == n_equ then
  message = "Check of" + modelName+" completed successfully.";
else
  message = "Model " + modelName + " is not balanced.";
end if;

The last case should be reported as an error, all the other ones as notifications.

perost commented 3 years ago

The first piece of information is there, so that's easy, just check if the two numbers are the same. The question is, can the NF return the second and third to CheckModel? In that case, we can add a last check to CheckModel, of course assuming all previous ones were successful:

Checking whether the model is partial is easy, checking if it contains any instances of partial classes less so (since they've been split into separate variables in the DAE).

casella commented 3 years ago

Can you just carry around a flag during flattening, and make it true when you encounter a partial class (instead of generating an error and quit)?

casella commented 2 years ago

Can you just carry around a flag during flattening, and make it true when you encounter a partial class (instead of generating an error and quit)?

@perost, I'm trying to figure out if we can get through this ticket or if we should just give up until we implement proper balancing checks in the future.

Would my last proposal be feasible, or is it too much work? I think understanding whether a certain class contains partial stuff or not could be valuable; in fact, we already do that during normal flattening (and abort in that case) and we're simply suppressing this check during CheckModel precisely because a model being checked doesn't necessarily need to be runnable.

What do you think?