The NF fails to evaluate expression in Buildings library

[Francesco Casella]

Please check
​Buildings.Fluid.Geothermal.Borefields.Examples.Borefields. The NF fails with

[Compiler/NFFrontEnd/NFCeval.mo:1125:9-1125:67:writable] Error: 
Internal error NFCeval.evalBinaryPow failed to evaluate
borFie2UTubParDat.conDat.hBor ^ 2.0

[Martin Sjölund]

borFie2UTubPar.groTemRes.borFieDat.conDat.hBor evaluates to borFie2UTubParDat.conDat.hBor which is a little odd.

[Martin Sjölund]

If I add a second evalExp_impl, it does evaluate it and gets further (but there are many other places where that cref is not evaluated fully).

[Per Östlund]

The issue is that the model has variability issues, where e.g. record instance parameter are given bindings that are variables, i.e. there are record instances that should be declared as parameter but aren't. There's been some confusion in the past about what the variability of record instances that only contain constants/parameters is, but ​#1832 says that there are no special rules for record instances.

So the error message comes from the fact that Ceval skips evaluating nonparameter crefs. Ceval should probably just give an error in such cases, but I think there's a reason for it that I don't remember. But it doesn't matter, the actual issue is that the variability issue should be caught earlier so we never get there in the first place.

I've fixed the variability check in ​PR 7361 so that we now get a better error message:

[Borefields.mo:2050:111-2050:140:writable] Error: Component borFieDat of variability parameter has binding 'borFie2UTubParDat' of higher variability continuous.

The fix is trivial, just add parameter to some of the components in the model. But I will wait until the library coverage job has been run with this change, in case there are other models in the library with this issue that should be fixed at the same time.

[Francesco Casella]

OK, it seems that we are on the right path to finally solve this issue and get a big boost of the success ratio of the library. Thanks!

[Francesco Casella]

I just checked the new error message, and indeed it worked fine

[Buildings 7.0.0/Fluid/Geothermal/Borefields/Examples/Borefields.mo:15:5-15:32:writable] 
Error: Component borFieDat of variability parameter has binding 'borFie2UTubParDat'
of higher variability continuous.

I'm not sure how many instances of this error are there in the Jenkins run of the library, which is important to know if you want to fix the library itself on the ​7.0.x maintenance branch. Maybe you can grep "of higher variability continuous" in the directory where all the error logs are written, in order to find out?

[Francesco Casella]

Unfortunately the other ticket failing because of not evaluated structural parameter, #5991, seems not to be affected, it probably has a different root cause.

[Per Östlund]

Replying to casella:

I'm not sure how many instances of this error are there in the Jenkins run of the library, which is important to know if you want to fix the library itself on the ​7.0.x maintenance branch. Maybe you can grep "of higher variability continuous" in the directory where all the error logs are written, in order to find out?

Already done, see ​this PR, so I guess we might as well close this.

[Francesco Casella]

I'd prefer to close this once we have proof everything works fine on our testsuite, just in case something goes wrong in the process.

BTW, I'm not sure I'm getting all the details right:

• @perost opened issue ​#2438
• @perost opened PR ​#2439 to address it, which was rejected by the CI system checks, maybe because it was applied to the master branch
• @mwetter then opened PR ​#2441 on master, which passed but is not yet merged in, and PR ​#2442 on maint_7.0.x, which is ready to be merged. I'm not sure if the two are equivalent, because they apparently refer to different commits, and if they both address the issue raised in ​#2438.

@mwetter, can you please shed some light and merge in everything ASAP, so we can see the results on our CI testsuite?

Thanks!

[Per Östlund]

Replying to casella:

I'd prefer to close this once we have proof everything works fine on our testsuite, just in case something goes wrong in the process.

Fine with me.

Replying to casella:

BTW, I'm not sure I'm getting all the details right:

• @perost opened issue ​#2438
• @perost opened PR ​#2439 to address it, which was rejected by the CI system checks, maybe because it was applied to the master branch
• @mwetter then opened PR ​#2441 on master, which passed but is not yet merged in, and PR ​#2442 on maint_7.0.x, which is ready to be merged. I'm not sure if the two are equivalent, because they apparently refer to different commits, and if they both address the issue raised in ​#2438.

@mwetter, can you please shed some light and merge in everything ASAP, so we can see the results on our CI testsuite?

Thanks!

I guess he can't reply here since the Trac is in read-only mode for non-admins, but the new PRs just apply my fix to master and the maintenance branch. I'm not sure why a new PR was needed for master, but it's just his workflow I guess.

[Francesco Casella]

OK, @mwetter please reply on github. We will migrate trac on github anytime soon :)

[Per Östlund]

Closing again since the library issue has now been fixed.

[Francesco Casella]

OK. Let's see how many models are fixed on our CI as soon as it runs again.

[Per Östlund]

Replying to casella:

OK. Let's see how many models are fixed on our CI as soon as it runs again.

15 models will be improved (since all the fixed models are example/validation models that don't affect other models), but about 6-7 of them still don't pass the frontend if I remember correctly from my testing.

[Per Östlund]

As predicted: ​https://libraries.openmodelica.org/branches/history/master/2021-04-09%2015:22:44..2021-04-10%2003:14:25.html

[Francesco Casella]

OK. Too bad many of them are now failing for other issues. We'll track them down later on.