BUG: apply wind factors to the ensemble member that was selected - #1160
Open
thc1006 wants to merge 1 commit into
Open
BUG: apply wind factors to the ensemble member that was selected#1160thc1006 wants to merge 1 commit into
thc1006 wants to merge 1 commit into
Conversation
select_ensemble_member() rebuilds the wind functions from the chosen member's own profile, and create_object() reached it after the factors because that is where __dict__ happened to put it. The factor was scaled into the previous member's wind and then thrown away, so the run flew the raw member wind while the input record still reported a factor. Factors are now applied once the loop is done, so the order of __dict__ stops mattering, and the baseline is the member just loaded rather than the value cached at construction. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1160 +/- ##
===========================================
+ Coverage 82.18% 83.56% +1.37%
===========================================
Files 122 130 +8
Lines 16355 17088 +733
===========================================
+ Hits 13441 14279 +838
+ Misses 2914 2809 -105 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Checklist
ruff check/ruff format --check) has passed locallypytest tests/unit tests/integration) have passed locallyCurrent behavior
StochasticEnvironment.create_object()walks the generated dictionary and, when it meetsensemble_member, callsselect_ensemble_member(). That call rebuilds the wind functions from the chosen member's own profile.ensemble_membersits inexception_list, so_set_stochastic()skips it and_validate_ensemble()sets it after the base constructor has already put the wind factors in__dict__. The factors are therefore reached first, scaled into whichever member was loaded at construction, and then overwritten.With two members whose winds are 10 and 30, member 1 selected and a factor of 2, the flight gets 30. The input record still says the factor was applied.
A determinism test would not catch this: the same seed is consistently wrong.
New behavior
Factors are collected during the loop and applied after it, so which entry
__dict__happens to hold first no longer decides the result. When a member was selected, the baseline is the profile that member just loaded; otherwise it is the construction-time value, so repeated calls still do not compound.The example above now gives 60.
Breaking change
Runs that combined an ensemble with a wind factor were silently ignoring the factor, so their results change. That was the bug.
Additional information
Three tests, filling the
test_validate_ensemblegap that was commented out intests/unit/stochastic/test_stochastic_environment.py:create_object()calls stay at 60 rather than compoundingReverting the fix turns the first two red; so does keeping the construction-time baseline after a member switch. The third stays green under both, which is what tells them apart.
Local run against
developat 1d04bcc: ruff clean, pylint exit 0,pytest tests/unit tests/integration2216 passed, 51 skipped.