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

Fixes 389 #390

Conversation

JulienGroenenboom
Copy link
Collaborator

No description provided.

@@ -139,7 +139,7 @@
refdate_str = 'minutes since '+ref_date+' 00:00:00 +00:00',
dir_output = dir_output,
list_quantities = list_quantities,
tstart=pd.Timestamp(date_min)-pd.Timedelta(hours=12), tstop=pd.Timestamp(date_max)+pd.Timedelta(hours=12), #TODO: to account for noon-fields of CMEMS, build in safety?
tstart=pd.Timestamp(date_min), tstop=pd.Timestamp(date_max),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also remove pd.Timestamp() around date_min and date_max

@@ -43,7 +43,7 @@ def preprocess_interpolate_nc_to_bc(ext_bnd,
else:
#open regulargridDataset and do some basic stuff (time selection, renaming depth/lat/lon/varname, converting units, etc)
data_xr_vars = dfmt.open_dataset_extra(dir_pattern=dir_pattern, quantity=quantity,
tstart=tstart, tstop=tstop,
tstart=pd.Timestamp(tstart)-pd.Timedelta(hours=12), tstop=pd.Timestamp(tstop)+pd.Timedelta(hours=12),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of several usecases where 12 hours would not suffice, for instance if you would have a model with tstop = '2020-12-01 23:00', the CMEMS tstop would be '2020-12-02 11:00', which is just too short of getting the noon field of that day. In this usecase we would also have to extend date_max in download_CMEMS(), I did not thought of that yesterday. Therefore, I have created the function dfmt.round_timestamp_to_outer_noon(). Could you update your branch and add a line like this in this function and in download_CMEMS()?:
tstart, tstop = dfmt.round_timestamp_to_outer_noon(tstart,tstop)

I also added a test for it, you could check the PR for the changes: https://github.com/Deltares/dfm_tools/pull/392/files

Comment on lines 135 to 137
#make sure the data fully covers the desired spatial extent. Download 1 additional grid cell (resolution is 1/12 degrees) in each direction
delta_latlon = 0.085 # which is slightly more than grid resolution (~1/12 degrees) #TODO: we could make use of ds.latitude.step/ds.longitude.step when the grid resolution is stored in a global
longitude_min, longitude_max, latitude_min, latitude_max = extend_domain(lon_min=longitude_min, lon_max=longitude_max, lat_min=latitude_min, lat_max=latitude_max, delta_latlon=delta_latlon)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, I suggest to replace these lines with:

#make sure the data fully covers the desired spatial extent. Download 2 additional grid cells (resolution is 1/12 degrees, but a bit more/less in alternating cells) in each direction
lon_min -= 2/12
lon_max += 2/12
lat_min -= 2/12
lat_max += 2/12

Comment on lines 64 to 65
#make sure the data fully covers the desired spatial extent. Download 1 additional grid cell (resolution is 1/4 degrees) in both direction
longitude_min, longitude_max, latitude_min, latitude_max = extend_domain(lon_min=longitude_min, lon_max=longitude_max, lat_min=latitude_min, lat_max=latitude_max, delta_latlon=1/4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to remove the extend_domain() function since and replace these lines with:

#make sure the data fully covers the desired spatial extent. Download 1 additional grid cell (resolution is 1/4 degrees) in both directions
lon_min -= 1/4
lon_max += 1/4
lat_min -= 1/4
lat_max += 1/4

I know it is against re-using code in functions, but this one is so simple and we hardcode delta_latlon anyway.

@sonarcloud
Copy link

sonarcloud bot commented Jun 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@veenstrajelmer veenstrajelmer merged commit c7f29a4 into main Jun 22, 2023
5 of 7 checks passed
@veenstrajelmer veenstrajelmer deleted the 389-slightly-extend-spatial-and-time-domain-when-downloading-era5-and-cmems-data branch June 22, 2023 10:31
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.

Slightly extend spatial and time domain when downloading ERA5 and CMEMS data
2 participants