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

Bugfix #138 in write_forcing #142

Merged
merged 8 commits into from Mar 23, 2023
Merged

Bugfix #138 in write_forcing #142

merged 8 commits into from Mar 23, 2023

Conversation

hboisgon
Copy link
Contributor

Correct bug with forcing with cftime.DatetimeNoLeap

Problem was coming that if forcing timespan was shorter than intented wflow starttime and endtime (old missings flag), we would sel the forcing dataset:

ds = ds.sel({"time": slice(start, end)})

The bug was here that start and end are pd.datetime and the slice does not work if ds.time is of type cftime.datetime. It can be fixed by converting start and end to strings first with start.strftime(format).
I don't really think we need the slicing though so I deleted it.

What I also found is that the starttime and endtime can have a different timestamp than the forcing (eg 2010-01-01 08:00:00 instead of 2010-01-01 00:00:00) so I added an extra check to correct the starttime and endtime to match better what is inside the forcing file

@hboisgon hboisgon self-assigned this Jan 10, 2023
@hboisgon hboisgon linked an issue Jan 10, 2023 that may be closed by this pull request
@hboisgon
Copy link
Contributor Author

@Jaapel can you check that this solves your issue ?

@Jaapel
Copy link

Jaapel commented Jan 11, 2023

@hboisgon I do not get the datetime error anymore 👍

@hboisgon
Copy link
Contributor Author

Would be nice to test hydromt_wflow (and hydromt) with cftime data maybe when cmip6 from gcs is implemented.
While digging I found than in xarray.open_dataset there is a use_cftime flag which is None by default. This means that times will be decoded to np.datetime64[ns] where possible and else to cftime.datetime:
https://docs.xarray.dev/en/stable/generated/xarray.open_dataset.html

As these two objects are quite different in functionalities I wonder if we maybe should not be more strict in hydromt and maybe always use cftime? @DirkEilander what do you think?

@DirkEilander
Copy link
Contributor

What are the different methods of the numpy datetime and cftime objects? I'm not sure if forcing all to be cftime will help or create more problems. Perhaps good to raise an issue in the core and investigate the benefits and limitations of both. If can easily accommodate for both that would be my preferred option.

@hboisgon
Copy link
Contributor Author

I agree it's not optimal to change but still maybe worth a discussion in HydroMT so I'll open one there. In the meantime, could you review this PR?

@laurenebouaziz
Copy link
Contributor

the workflow does not work for 360 days calendars.
when da.indexes['time'].calendar != "360_day" the .to_datetimeindex() does not work because of the presence of 29th of february each year:
*** ValueError: Cannot convert date 1970-02-29 00:00:00 to a date in the standard calendar. Reason: day is out of range for month.

Maybe good to further discuss how we want to deal with this.

@laurenebouaziz
Copy link
Contributor

discussed with @hboisgon that we only correct dates in toml for standard calendars. see 0b3f977

@laurenebouaziz laurenebouaziz merged commit 36c95ec into main Mar 23, 2023
1 of 2 checks passed
@laurenebouaziz laurenebouaziz deleted the bug/138_datetime branch March 23, 2023 10:44
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.

Error when creating forcing using datetime noleap type
4 participants