Add missing automatic_{profile,trend}_frequency parameter #252
Conversation
Codecov Report
@@ Coverage Diff @@
## rb-ASIM-v22.2.0 #252 +/- ##
================================================
Coverage 99.95% 99.95%
================================================
Files 41 41
Lines 4457 4457
================================================
Hits 4455 4455
Misses 2 2 |
| @@ -1592,8 +1592,10 @@ def load_case_output_description( | |||
| ) -> case_description.CaseOutputDescription: | |||
There was a problem hiding this comment.
Should I add a unit test for this function somewhere?
There was a problem hiding this comment.
I believe it is already tested since you just added those new parameters to the filled_case_description.
There was a problem hiding this comment.
You'd think that, but the tests pass regardless of the two fields added to this dict.
There was a problem hiding this comment.
I would say "don't add test for this" because I think all explicit loaders should die =) #216
| @@ -1592,8 +1592,10 @@ def load_case_output_description( | |||
| ) -> case_description.CaseOutputDescription: | |||
There was a problem hiding this comment.
I believe it is already tested since you just added those new parameters to the filled_case_description.
| @@ -441,8 +441,10 @@ | |||
| ) | |||
| CASE_OUTPUT_DESCRIPTION = case_description.CaseOutputDescription( | |||
| trends=TRENDS_OUTPUT_DESCRIPTION, | |||
| automatic_trend_frequency=True, | |||
There was a problem hiding this comment.
What is the idea of these parameters? If they are True, the trend_frequency and profile_frequency are ignored?
There was a problem hiding this comment.
They already exist, but are missing in some places. If I understand them correctly, it's like what you said, automatic_... being True means the property will be derived automatically from some defaults.
From the application's tooltips:
Uses an automatically defined frequency to save profile output data.
The frequency used is the highest value between: default profile frequency, maximum timestep configured, and an estimate based on simulation time (1/10 of total time).
There was a problem hiding this comment.
Nice. Thanks for the explanation!
There was a problem hiding this comment.
It is also possible that the "case description"-"alfacase" round trip is saving the unused values. I remember to add that option with the multi inputs to test transient and constant in one go (but disabled on production).
But as he pointed out "... used is the highest value between: default profile frequency, maximum timestep configured, and an estimate ..."
No description provided.