Skip to content

improved ill-posed integer rounding#2089

Open
JayHuLBL wants to merge 10 commits intomasterfrom
issue2088_illPosed_integer_rounding
Open

improved ill-posed integer rounding#2089
JayHuLBL wants to merge 10 commits intomasterfrom
issue2088_illPosed_integer_rounding

Conversation

@JayHuLBL
Copy link
Contributor

This closes #2088.

Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still has potential rounding issues.

Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now.

@mwetter mwetter requested a review from jelgerjansen February 24, 2026 13:50
@mwetter
Copy link
Contributor

mwetter commented Feb 24, 2026

@jelgerjansen : can you please provide a review so we have a non-LBL participant approving it. Thanks.

Copy link
Contributor

@jelgerjansen jelgerjansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JayHuLBL thanks for looking into this.

I noticed that you still applied some changes (commit 10f3e56) since Michael's approved review (commit 8462d07). Why did you split the initialization event, which is now still using the integer() implementation, from the runtime events, using the numerically more robust implementation?

Additionally, can you also:

  1. Move the documentation (lines 23-24) to the correct part of the code (modTimAux > pre(tNext))?
  2. Refer to the IBPSA issue next to the Buildings issue in the review notes (for completeness)?

@JayHuLBL
Copy link
Contributor Author

@jelgerjansen The changes done in commit # 10f3e56 is to consider case when the simulation starts from the second half of the year.

For instance, if a simulation's start time is 190 days, then floor(modTimAux/lenWea + 0.5)*lenWea + lenWea will be 2*lenWea, which should be 1*lenWea.

Thanks for other comments. I will change it accordingly.

@mwetter
Copy link
Contributor

mwetter commented Feb 26, 2026

@JayHuLBL : I still don't understand why there are two different equations to assign tNext.
If there is indeed a rational, it must be documented in the .mo file so users and developers understand.
Also, is the start attribute needed for tNext if it is assigned in an initial() clause?

@JayHuLBL
Copy link
Contributor Author

The changes here is to address the potential issue that when the event is triggered due to the condition canRepeatWeatherFile and modTimAux > pre(tNext)) (thus for example modTimAux/lenWea = 1.00001), there is potential that it is triggered a little bit earlier (as described in (lbl-srg/modelica-buildings#4480) (thus for example modTimAux/lenWea = 0.99999). By changing the floor(modTimAux/lenWea) to floor(modTimAux/lenWea + 0.5) will ensure the right tNext calculation.

However, for the initial event, it is clear that it is triggered at the right time and it is safe to calculate tNext as floor(modTimAux/lenWea)*lenWea + lenWea.

@JayHuLBL
Copy link
Contributor Author

JayHuLBL commented Mar 2, 2026

@mwetter The implementation has been further improved through commit 4b45e33. Would you please review if the change looks good?

Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment.

// (last time stamp of the weather file + average increment)
when (canRepeatWeatherFile and modTimAux > pre(tNext)) then
k = pre(k) + 1;
tNext = if canRepeatWeatherFile then k * lenWea else time;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there an if-then-else needed? If we are here, then canRepeatWeatherFile must be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in commit ecffbe9.

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 this pull request may close these issues.

Ill-posed integer rounding

3 participants