-
Notifications
You must be signed in to change notification settings - Fork 31
gridded: dimensions and variable exclusion #227
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
base: main
Are you sure you want to change the base?
Conversation
…_direction as the mode
@jklymak a couple fyis to explain my thinking. Happy to adjust these as you like. Sorry, not sure how to directly embed lines from the pr in the comment
Changing this from
Profile direction felt useful to add, as an indicator of if a profile was while the glider was ascending or descending. Since it was only binned by profile, it seemed easiest to put direction with the time/lat/lon binning
It felt simplest to let the user pass a list of variables (exclude_var) to ignore when gridding, rather than eg looking for a new 'grid_ignore: true' key in the deploymentyaml file. This line, along with the check
|
One other question. Is there a reason that |
@jklymak sorry for the ping, but I wanted to check in about both this question, and the pr as a whole? |
exclude_vars += list(dsout.keys()) + ["depth"] | ||
for k in ds.keys(): | ||
if (k in exclude_vars) or ('time' in k) or ('profile' in k): | ||
_log.debug('Not gridding %s', k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be gridding longitude and latitude by deafult now? Or are they in the default exclude_vars
?
I wonder if for non-coordinate data (eg temperature etc) we should really be setting whether to grid it or not in the yaml instead of specifying a list here. eg in the yaml say grid_exclude: True
if we don't want it to be gridded, but we do want it in the time series?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be gridding longitude and latitude by deafult now? Or are they in the default
exclude_vars
?
longitude and latitude (along with time, profile_direction, profile_time_start, and profile_time_end) are all guaranteed to be part of dsout. Thus, they're always added to exclude_vars
in exclude_vars += list(dsout.keys()) + ["depth"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if for non-coordinate data (eg temperature etc) we should really be setting whether to grid it or not in the yaml instead of specifying a list here. eg in the yaml say
grid_exclude: True
if we don't want it to be gridded, but we do want it in the time series?
A grid_exclude
boolean key feels totally reasonable to me. It's also more consistent with the function checking for the 'average_method' key. I'll update the pr now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this seems good. I was just wondering though if we wanted to allow the user to exclude a timeseries variable from being gridded at this stage by adding a property to the yaml. But maybe that can be a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smwoodman I was waiting to understand this...
@jklymak wanted to check in about this, as I'm currently working on some code that uses these features. But, please please let me know if you're busy at the moment, and prefer that either I ping someone else or we pick this back up at a later date! |
Addresses #226