-
Notifications
You must be signed in to change notification settings - Fork 58
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
screen limiter #1290
Comments
@Mathadon what would be the main reason to change to an assert? |
Both! It adds computations that serve no use if the models are built up correctly. The min/max is evaluated on each function call even while iterating on the implicit solver, while an assert should only be evaluated after convergence of the implicit solver, which is consequently much less frequently. |
@jelgerjansen @Mathadon I just had a look into this issue. I guess we can simply remove the limiter block without adding an assertion warning/error. The limiter block is used in IDEAS.Buildings.Components.Shading.Screen and is used for the Ctrl input connector.
Hence, if I remove the limiter block in the screen model, I still get an error if I use a ctrl input smaller than 0 or greater than 1. So I suggest removing the limiter block, without adding an assert statement. is this fine for you |
@lucasverleyen I don't think you will get this error if you connect a non-constant to the input. So you'd need an assert to cover that case. |
@lucasverleyen if you run |
@Mathadon @jelgerjansen good catch! I'll add the assert statement. |
For completeness and according to @jelgerjansen's good suggestion, I have checked the results in the |
@jelgerjansen to follow up on your comment. I tried to add the assert statement to the |
@lucasverleyen a solution for the problem you described could be circumvented by introducing an internal input
@Mathadon what do you think? I think it makes more sense to integrate the assert in the partial model to guard against unrealistic control inputs for all screen models. |
@jelgerjansen I have implemented the changes as discussed.
However, the HorizontalFins model used the Ctrl input for the inclination angle of the fins, i.e. a value between 0 and 90° or 0 and 1.57, which violates the assert statement in the partial model. Hence, I have modified the HorizontalFins model by adding an internal variable (Ctrl_to_beta_internal) to linearly map the Ctrl input [0,1] onto the fin inclination angle [beta_min=0,beta_max]. I had to add a new internal variable because of the conditional declaration of Ctrl. If Ctrl is used, it is always a value between 0 and 1 for any shading model. Consequently, I have to update the reference results for HorizontalFinExample because the value of the Ctrl variable is used and has changed. @Mathadon FYI |
The reference results
Only the reference results of |
@lucasverleyen I further updated
If you think something is still wrong, we can reopen this issue. |
remove limiter block in screen. This closes #1290
Hi, Due to the changes to the PartialShading and Screen model I now get an error using OCT when simulating with screens:
I believe that instead of using Ctrl in the Screen model it should be replaced by Ctrl_internal |
Shouldn't we use an assert instead of this:
?
The text was updated successfully, but these errors were encountered: