Clarify time handling (+bugfix) #352
Conversation
|
This PR:
Working in hydrological years might be confusing at first but this is what we want for the model output. With this format it is now possible to compute annual averages based on both calendars. The recommended way is simply: |
|
cc @jlandmann |
| ---------- | ||
| hydro_year : bool | ||
| If the float year follows the "hydrological year" convention or | ||
| not (default:True) |
There was a problem hiding this comment.
It might be nice to include in the description that y1 or yn need to be defined. On the other hand it is very clear when you read the function how it works, so it might not be really needed.
There was a problem hiding this comment.
Yes you are right the docstring is not complete here
| @@ -238,6 +238,7 @@ def __init__(self, gdir, mu_star=None, bias=None, prcp_fac=None, | |||
| ny, r = divmod(len(time), 12) | |||
| if r != 0: | |||
| raise ValueError('Climate data should be N full years') | |||
There was a problem hiding this comment.
Maybe it is a good moment to also include checking if the climate data actually start in January.
There was a problem hiding this comment.
Actually, we assume the that the data starts in October (because of the hydrological year)
There was a problem hiding this comment.
Somehow I mixed things up, but you are right.
oggm/core/flowline.py
Outdated
| diag_ds['ela_m'] = ('time', np.zeros(nm) * np.NaN) | ||
| diag_ds['ela_m'].attrs['description'] = ('Annual Equilibrium Line ' | ||
| 'Altitude') |
There was a problem hiding this comment.
I think that the Equilibrium Line Altitude is by definition annual, therefore I believe that the word 'annual' is redundant in this case.
There was a problem hiding this comment.
Correct. I'll keep it that way though: because of implementation details, it is still stored at a monthly time step, so to make it clear I added the "Annual"
There was a problem hiding this comment.
I didn't think of that, but in that case it makes indeed a lot off sense to leave it like it is :)
|
Thanks for the review! Next time I'll try to wait a bit more before merging ;) |
|
Thanks @fmaussion for making the changes, it made things more clear to me :) |
closes #348