Limit the maximum pre-period end date to upgrade date#88
Limit the maximum pre-period end date to upgrade date#88samuelwnaylor wants to merge 4 commits intomainfrom
Conversation
If the `analysis_last_dt_utc_start` is >1 year post upgrade date then ensure that if the `years_offset_for_pre_period` is specified as 1 year, then the maximum end date of the pre-period is the upgrade date (with 1 day as contingency).
samuelwnaylor
force-pushed
the
pre-date-max-limit
branch
from
1f9b46e to
aefdc7e
Compare
aclerc
left a comment
aclerc
left a comment
There was a problem hiding this comment.
Thanks Sam, this is nice validation! A few comments, happy to discuss
wind_up/models.py
Outdated
| if pre_last_dt_utc_start > cfg_dct["upgrade_first_dt_utc_start"]: | ||
| pre_last_dt_utc_start = pd.to_datetime(cfg_dct["upgrade_first_dt_utc_start"]) - pd.Timedelta( | ||
| days=1 | ||
| ) # One day for contingency |
There was a problem hiding this comment.
I'd take out the one day for contingency. That way this simplifies to pre_last_dt_utc_start being clipped to an upper value of cfg_dct["upgrade_first_dt_utc_start"]. Otherwise pre_last_dt_utc_start is allowed to be as late as cfg_dct["upgrade_first_dt_utc_start"], but then as soon as it goes over it gets shoved back a day which seems inconsistent
another thought is that we want pre and post to not contain the same timestamps, so really we want to clip pre_last_dt_utc_start to be no later than upgrade_first_dt_utc_start minus the analysis timebase. If you change that here then I think the inconsistency with validation is fixed (other comment).
| """Check that the pre-period does not extend over the upgrade date. | ||
|
|
||
| Check that if the `analysis_last_dt_utc_start` is >1 year post upgrade that if the `years_offset_for_pre_period` is | ||
| 1 year, then the maximum end date of the pre-period is the upgrade date (with a days contingency). |
There was a problem hiding this comment.
as per other comments I'd set the pre period to the upgrade timestamp minus the analysis timebase
aclerc
left a comment
aclerc
left a comment
There was a problem hiding this comment.
looking good! just some minor requests
wind_up/models.py
Outdated
| if pre_last_dt_utc_start > cfg_dct["upgrade_first_dt_utc_start"]: | ||
| pre_last_dt_utc_start = pd.to_datetime(cfg_dct["upgrade_first_dt_utc_start"]) - pd.Timedelta( | ||
| seconds=cfg_dct.get("timebase_s", DEFAULT_TIMEBASE_S) | ||
| ) |
There was a problem hiding this comment.
I think here you should test for greater than the value you are clipping it to, pd.to_datetime(cfg_dct["upgrade_first_dt_utc_start"]) - pd.Timedelta(seconds=cfg_dct.get("timebase_s", DEFAULT_TIMEBASE_S). What a great walrus opportunity
samuelwnaylor
force-pushed
the
pre-date-max-limit
branch
from
fc6dc3d to
6aafceb
Compare
2 participants
If the
analysis_last_dt_utc_startis >1 year post upgrade date then ensure that if theyears_offset_for_pre_periodis specified as 1 year, then the maximum end date of the pre-period is the upgrade date (with 1 day as contingency).