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

Automatically call griddap_initialize if a dataset_id exists #353

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Aug 29, 2024

Right now we need this extra step for griddap but we can skip it run that automatically every time a dataset_id is set.

@ocefpaf
Copy link
Member Author

ocefpaf commented Aug 29, 2024

@callumrollo this is something I wanted to do for a long time but only got to it after your tutorial yesterday.

The PR is not ready yet b/c we need to think about the possible corner cases. Also, setting a griddap dataset_id is slow now b/c griddap_initialize is always called. I'm not sure if there is a better option b/c we allow users to re-call it, making checking if those properties are already defined would prevent re-running... Or we can make a breaking change and throw away re-running griddap_initialize and assume that, if self.constraints, self.dim_names, and self.variables, calling griddap_initialize won't change them.

@ocefpaf ocefpaf force-pushed the auto_init_griddap branch 2 times, most recently from d29684a to a1ed1a4 Compare August 29, 2024 11:40
@callumrollo
Copy link
Contributor

Nice! I think calling griddap_initialize in the background when a user sets dataset_id when the server was set up with protocol=griddap is a great idea. I don't think the delay is significant enough to be annoying.

I would recommend against throwing away the user available griddap_initialize method though. As a user I sometimes do something like:

e = ERDDAP(y)
e.dataset_id = x
e.griddap_initialise (no longer need this, nice!)
e.variables = a,b,c
e.constraints = {} # some breaking changes
e.to_xarray() # fails due to the breaking changes I made
e.griddap_initialize() # reset the constraints to something that works again

@ocefpaf
Copy link
Member Author

ocefpaf commented Aug 30, 2024

I thought about renaming it to griddap_reset or re_initialize but that would be a breaking change. We will keep it and document that one should use it only to "reset the query params."

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