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

use the ERDDAP server instead or pyoos #88

Merged
merged 5 commits into from
Jun 1, 2022
Merged

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented May 27, 2022

Thank you for send a Pull Request to our code gallery! When adding or updating a notebook please check if:

  • The notebook has all the dependencies required to run in the IOOS env, if not please update the environment file.
  • You added a title, description, and a line with Created: YYYY-MM-DD in the first cell.
  • If you are updating a notebook add a line with Updated: YYYY-MM-DD below the created date.

Closes #46

@MathewBiddle note that I read the station info from the XML using some xpath tricks.That really defeats the purpose of having that data in ERDDAP b/c one cannot filter using just ERDDAP and some extra Knowledge of the data is requires before hand . I'm not sure why that server is set up the way it is, looks like some metadata may be missing or the original data format is not ideal*. It is still better than pyoos and the old SOS.

* I would get rid of BEGIN_DATE and END_DATE variables, the time variable should be enough. I would also change STATION_ID from a variable to a dataset_id. The current dataset_ids are what should be variables, like Water Level - Verified Six Minute.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ocefpaf ocefpaf marked this pull request as ready for review May 27, 2022 14:07
@ocefpaf ocefpaf requested a review from MathewBiddle May 27, 2022 14:36
@ocefpaf ocefpaf mentioned this pull request May 27, 2022
@ocefpaf
Copy link
Member Author

ocefpaf commented May 30, 2022

@MathewBiddle this one is ready for review. I removed the QA/QC part b/c that no longer makes sense. The data was corrected in the server and the cleanup step we had here is no longer necessary.

Copy link
Contributor

@MathewBiddle MathewBiddle left a comment

Choose a reason for hiding this comment

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

I've made some adjustments to the narrative and added a map plot of the bounding box, just to set the scene a little more.

I was trying to figure out where that stationsXml opendap link came from and I can't trace it back. Could you add something to that section describing where you got it?

Also, it would be nice to show the ERDDAP request that fails as you describe in the beginning. Is there anyway we could show how it fails and what we have to do to work around it? This could be a nice example of how ERDDAP admins could make things difficult for users.

@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 1, 2022

I've made some adjustments to the narrative and added a map plot of the bounding box, just to set the scene a little more.

👍

I was trying to figure out where that stationsXml opendap link came from and I can't trace it back. Could you add something to that section describing where you got it?

That one was not easy to find. After multiple failed attempts I found a link in the metadata for STATION_ID that sends me to a page where we can view the stations and download the XML file.

stationsXML

I'll add a cell that prints the STATION_ID metadata and some text to explain how to get the xml from it.

Also, it would be nice to show the ERDDAP request that fails as you describe in the beginning. Is there anyway we could show how it fails and what we have to do to work around it? This could be a nice example of how ERDDAP admins could make things difficult for users.

It fails silently if we add longitude, latitude, or time. That is worse IMO b/c one will think it is working and only realize they are missing a key info when they try to build a URL without the STATION_ID. BTW, depending on the request, even after adding the STATION_ID one could think that the constraints worked if by luck they chose a STATION_ID inside them.

TL;DR I'd rather put another notebook where we can show the problem to the data providers instead of adding many extra steps here just to show a failed attempt to users. I believe users should never see that and, IMO, that server is kind of broken.

@ocefpaf ocefpaf requested a review from MathewBiddle June 1, 2022 12:51
Copy link
Contributor

@MathewBiddle MathewBiddle left a comment

Choose a reason for hiding this comment

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

Just needed to update the title SOS -> ERDDAP

Looks good to merge!

@ocefpaf ocefpaf merged commit 785973e into ioos:main Jun 1, 2022
@ocefpaf ocefpaf deleted the COOPS_ERDDAP branch June 1, 2022 19:59
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.

Update CO-OPS SOS python notebook
2 participants