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

keeping masks as nc variables #92

Open
ChrisBarker-NOAA opened this issue Oct 31, 2024 · 8 comments
Open

keeping masks as nc variables #92

ChrisBarker-NOAA opened this issue Oct 31, 2024 · 8 comments

Comments

@ChrisBarker-NOAA
Copy link
Contributor

I see this in sgrid: get_masks

                node_mask = self.nc.variables[mc] #do not [:] mask variables. They contain important metadata

This is a problem -- once in the SGRid class, attributes may nor may not be netcdf variables -- we REALLY don't want the main class to be tied to netCDF like that (they should all be "array like'. So if there's metadata we need (what is it?) then that should be stored somewhere in the SGRrid class, and not expected to be in the array itself.

@jay-hennen: we need to be careful about this -- how/where is this tested?

There should be SGRid (and everything else) test that don't involve creating it from a netCDF file.

@jay-hennen
Copy link
Contributor

The metadata would be the flag_meanings and flag_values. The API assumption in this case isn't netCDF specifically. It's that you can access flag_meanings and flag_values directly eg: node_mask.flag_meanings

@ChrisBarker-NOAA
Copy link
Contributor Author

yes, but if flag_values is an arbitrary numpy array-like it won't have attributes -- those should be stored iin some other way.

e.g. extract them from the netcdf variable when you load, then set them as a attribute on the object.

@jay-hennen
Copy link
Contributor

If a mask var doesn't have specific attributes (via hasattr) then the code just uses the mask as is. I suppose it could look for these in a separate dict or something instead. grid.node_mask_attrs['flag_meanings'] or something?

If you haven't seen it already, this is function that processes masks from their 'raw' form into a 'true/false' mask.

def convert_mask_to_numpy_mask(mask_var):

@ChrisBarker-NOAA
Copy link
Contributor Author

OK then -- the question is where you drawn the line -- I would think that a fully populated and configured SGrid object would have the numpy mask -- so convert_mask_to_numpy_mask should be called in the netcdf-specific code, and then set the attribute with the numpy mask.

@ChrisBarker-NOAA
Copy link
Contributor Author

hmm -- looking now, I see convert_mask_to_numpy_mask defined in gridded, but I dont see it called anywhere. not even in PyGNOME.

@jay-hennen
Copy link
Contributor

It's only used in one place. I admit I didn't expect when I originally wrote the encapsulation so that's why it is the way it is. It's used to mask off parts of the celltree, because masked portions of the grid can contain 'messed up' lat/lon

def gen_celltree_mask_from_center_mask(center_mask, sl):

@ChrisBarker-NOAA
Copy link
Contributor Author

OK -- I see it now -- not sure why my grepping didn't find it before.

But I think this means that when gen_celltree_mask_from_center_mask is called, the mask should already be a numpy array, so that conversion doesn't need to happen here.

What is the sl parameter to that function? -- not obvious to me when I look at the caller.

I'm vary wary of changing any of this, though, 'cause I dont know how well it's tested....

@ChrisBarker-NOAA
Copy link
Contributor Author

Hmm -- thinking now, I think maybe, at the SGRid class level, I think maybe should have:

  1. A separate array for each mask -- e.g. "land_mask" (or a more generic name -- e.g. "grid_mask" -- that would be a mask for which grid cells are valid.

  2. maybe arrays for other masks -- but do we need them?

(note: they could be properties, and auto-generated from a single array with different mask values ....

In any case, the ROMS netcdf file specific specification should be outside the SGrid class.

I'm going to perhaps look at this more if/when I figure out Dylan's ROMS problem ....

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

No branches or pull requests

2 participants