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

change radiation of weather data to exactly zero rather than 1E-4 #1340

Closed
mwetter opened this issue Apr 10, 2020 · 8 comments · Fixed by #1344
Closed

change radiation of weather data to exactly zero rather than 1E-4 #1340

mwetter opened this issue Apr 10, 2020 · 8 comments · Fixed by #1344

Comments

@mwetter
Copy link
Contributor

mwetter commented Apr 10, 2020

The weather data reader always produces slightly positive radiation values. Looking at WeatherData.Examples.ReaderTMY3, one can see that if the radiation is set to 0, then HDirNor, HGloHor and HDifHor are 0.0001 W/m2. We set them to a slightly positive value because time interpolation of weather data can lead to slightly negative values, and the smoothMax was used to avoid events. In the meantime, MSL ensures that max does not generate events. This issue is to test if we could set these values to exactly 0 and avoid using the smoothMax here.

This will need to be tested on larger models, e.g., as part of the Buildings library.

The reason is that non-zero values do not allow achieving steady-state in a building model, which a user tried to accomplish as part of controls design.

@mwetter
Copy link
Contributor Author

mwetter commented Apr 14, 2020

This refactoring should also replace code such as

  assert(TOut > TMin, "In " + getInstanceName() + 
": Weather data dry bulb temperature out of bounds.\n" + "   TOut = " + String(TOut));

to add noEvent because these statements cause a zero crossing function to be generated by OPTIMICA.

@Mathadon
Copy link
Member

This refactoring should also replace code such as

  assert(TOut > TMin, "In " + getInstanceName() + 
": Weather data dry bulb temperature out of bounds.\n" + "   TOut = " + String(TOut));

to add noEvent because these statements cause a zero crossing function to be generated by OPTIMICA.

Is that necessarily a bad thing?

mwetter added a commit that referenced this issue Apr 14, 2020
For #1340. [ci skip]
@mwetter
Copy link
Contributor Author

mwetter commented Apr 14, 2020

In my view there is no need to have the FMU generate a zero crossing function and the solver having to monitor it to see if an event happened. (And for QSS there will be an overhead because it approximates these to predict when an event happens.)

mwetter added a commit that referenced this issue Apr 14, 2020
@Mathadon
Copy link
Member

Correct me if I'm wrong:
zero crossing functions are used to determine when exactly an event happened. I.e. the solver first checks whether a state event happened, if so, backtracks to see when it exactly happened. I presume that in the case of asserts, we do not really care when the event exactly happened. Although in some cases, one event may happen before another event, and not doing the backtracking may cause the events to be reported in different orders, which makes it harder to debug what event happened first.

Conclusion: I indeed see no point in doing event tracking for this particular assert, or even in general for our asserts. But there could be a use case, so it wouldn't make sense to disable event generation for asserts all-together in Optimica. This could however still be an optional feature request for Optimica. It should not take more than an hour of work to implement this (excluding the work on the CI-side).

@mwetter
Copy link
Contributor Author

mwetter commented Apr 15, 2020

@Mathadon zero crossing functions are used not only to determine when exactly an event happened, but also whether an event happens. This is the information that a solver has. I found the FMI specification quite useful to understand how zero crossing functions are used.

I think best is to decide on a case by case basis whether an event is useful. For checking for extreme (out of range) temperatures, I think there is no use as other parts of the model typically still keep computing. If this assertion comes a bit late, that is fine in my view.

mwetter added a commit that referenced this issue Apr 18, 2020
@mwetter
Copy link
Contributor Author

mwetter commented Apr 18, 2020

This refactoring also corrects the name of the weather data bus variable
weaBus.celHei to weaBus.ceiHei

@mwetter
Copy link
Contributor Author

mwetter commented Apr 21, 2020

All tests of this refactored weather data reader work in IBPSA and Buildings, and the computing time did not change much, see attached zip file.

branches_compare_dymola.zip

@Mathadon
Copy link
Member

All tests of this refactored weather data reader work in IBPSA and Buildings, and the computing time did not change much, see attached zip file.

branches_compare_dymola.zip

this looks ok

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

Successfully merging a pull request may close this issue.

2 participants