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

Nemo 3d #463

Merged
merged 15 commits into from
Oct 3, 2018
Merged

Nemo 3d #463

merged 15 commits into from
Oct 3, 2018

Conversation

delandmeterp
Copy link
Contributor

No description provided.

parcels/field.py Outdated
indices = filebuffer.indices
# Check if parcels_mesh has been explicitly set in file
if 'parcels_mesh' in filebuffer.dataset.attrs:
mesh = filebuffer.dataset.attrs['parcels_mesh']

dimension_filename = filenames[0]
with NetcdfFileBuffer(dimension_filename, dimensions, indices) as filebuffer:
depth = filebuffer.read_depth
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a very pretty solution, I'd say. This only works for nemo, but may leave users of other datasets out. Even if they would put depth in their dimension_filename, that would be overloaded here. Should we implement this in a way that is more flexible for different use cases, not only nemo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we replace the mesh_mask file which is not super explicit by a dimesion_file, which can be a file (the mesh_mask) or a dict of files, per dimension.
This follows the logic as everywhere else, no?

Copy link
Member

Choose a reason for hiding this comment

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

yes, that is a good idea. Although we need to support also that dimensions can also be a dictionary of dictionaries, for each variable. So then we could have two dictionaries: dimension_names and dimension_files.

The former is what we now have as dimensions, and the latter would default to filenames[0] (i.e. assume that all dimensions information is simply in the variable files) but can be overloaded as either a single dictionary (dimension_files['lon']) or a dictionary-of-dictionaries (dimension_files['U']['lon']).
Makes sense?

" 'data': data_path + 'U_purely_zonal-ORCA025_grid_U.nc4'},\n",
" 'V': {'lon': data_path + 'mesh_mask.nc4',\n",
" 'lat': data_path + 'mesh_mask.nc4',\n",
" 'data': data_path + 'V_purely_zonal-ORCA025_grid_V.nc4'}}\n",
Copy link
Member

Choose a reason for hiding this comment

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

Does this new API for filenames also need to be documented? Perhaps explain here in the nemo tutorial, and also add in the docstring of FieldSet.from_netcdf etc?

parcels/field.py Outdated
data_filenames = filenames['data'] if type(filenames) is dict else filenames
if type(filenames) == dict:
assert len(filenames['lon']) == 1
assert filenames['lon'] == filenames['lat']
Copy link
Member

Choose a reason for hiding this comment

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

why can't 'lon' and 'lat' not be in different files? Seems too strict to me? Or, if we need to be this strict, then at least add an error message

raise IOError("FieldSet files not found: %s" % str(notfound_paths))
for fp in paths[dim]:
if not path.exists(fp):
raise IOError("FieldSet file not found: %s" % str(fp))
Copy link
Member

Choose a reason for hiding this comment

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

Here, we have two copies of almost the same code. Create a function instead, for conciseness an ease of maintenance?

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Minor suggestions for clarifying docstrings

Do we also need to add comments about filename structure to Field.from_netcdf?

@@ -33,7 +33,17 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"We can create a `FieldSet` just like we do for normal grids. Note that we need to provide an extra filename for the `mesh_mask` file that holds information on the Curvilinear grid."
"We can create a `FieldSet` just like we do for normal grids.\n",
"Note that NEMO is discretised on a C-grid. U and V velocities are not located on the same grid (see https://www.nemo-ocean.eu/doc/node19.html).\n",
Copy link
Member

Choose a reason for hiding this comment

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

Is 'grid' the right word here, is 'not located on the same grid'? Perhaps 'location'?

"|__V0__|\n",
"```\n",
"\n",
"To interpolate U, V velocities on the C-grid, Parcels needs to read the f-nodes, which are located on the corners of the cells."
Copy link
Member

Choose a reason for hiding this comment

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

change to 'lower-left corners' to be more explicit which corner?

@@ -160,6 +160,9 @@ def from_netcdf(cls, filenames, variables, dimensions, indices=None,
:param filenames: Dictionary mapping variables to file(s). The
filepath may contain wildcards to indicate multiple files,
or be a list of file.
filenames can be a list [files], a dictionary {var:[files]},
a dictionary {dim:[files]} (if lon, lat, depth not stored in same files as data),
a dictionary of dictionaries {var:{dim:[files]}}
Copy link
Member

Choose a reason for hiding this comment

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

add 'or' before the dictionary-of-dictionaries?

or be a list of file.
filenames can be a list [files], a dictionary {var:[files]},
a dictionary {dim:[files]} (if lon, lat, depth not stored in same files as data),
a dictionary of dictionaries {var:{dim:[files]}}
Copy link
Member

Choose a reason for hiding this comment

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

see comment above

U0 U1
|__V0__|
To interpolate U, V velocities on the C-grid, Parcels needs to read the f-nodes,
which are located on the corners of the cells.
Copy link
Member

Choose a reason for hiding this comment

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

see minor comments on the jupyter notebook

@delandmeterp delandmeterp merged commit 2368add into master Oct 3, 2018
@delandmeterp delandmeterp deleted the nemo_3d branch October 3, 2018 12:26
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.

2 participants