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

Issue1912 avoid getPeak function unless asked #1923

Merged
merged 7 commits into from
Aug 28, 2024

Conversation

hcasperfu
Copy link
Contributor

This closes #1912.

@hcasperfu hcasperfu requested a review from FWuellhorst August 21, 2024 01:21
@hcasperfu hcasperfu self-assigned this Aug 21, 2024
Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

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

Code Changes look good, but I don't get why the reference results change as you mentioned that the unused default should not impact the results.

@hcasperfu
Copy link
Contributor Author

hcasperfu commented Aug 21, 2024

@FWuellhorst -
Thanks for the review!
In short, the affected validation models explicitly used the per.peak parameter. In the old code, it was always obtained from the getPeak() function call and would be the same. In the new code, the function call is avoided unless needed, causing each mover component to potentially have different values in per.peak. Hence the validation models were refactored to have a standalone peak parameter so that they are still making valid comparisons. This caused slight changes in the results. Without the refactoring the results would be drastically different and simply wrong.

In addition, there is an unrelated fail in CI which I think could be resolved if the test is rerun.

Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

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

OK good, thanks for the explanation!

@hcasperfu hcasperfu requested a review from mwetter August 23, 2024 15:46
@mwetter mwetter merged commit 6a19f22 into master Aug 28, 2024
3 checks passed
@mwetter mwetter deleted the issue1912_avoidGetPeakFunctionUnlessAsked branch August 28, 2024 00:22
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.

Root error in getPeak calculation even though Euler is not selected
3 participants