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

loading ECCO v4r4 in zarr format #200

Merged
merged 25 commits into from
Feb 14, 2022
Merged

Conversation

asiddi24
Copy link
Collaborator

I've added the ECCO v4r4 intake catalog entry and removed the test data. Grid is not added separately since the dataset was stored with the grid added to it in the merged dataset. Added grid arguments though to the catalog entry.

The data loads super quick with the zarr format, granted it's about 145 GB. Following up on #168, there needs to be something of the sorts xarray_kwargs in the xr.open_zarr functionality.

All looks good. Will update #168 with changes as well.

@pep8speaks
Copy link

pep8speaks commented Sep 21, 2021

Hello @asiddi24! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 171:1: W293 blank line contains whitespace

Comment last updated at 2022-02-14 20:47:07 UTC

@asiddi24
Copy link
Collaborator Author

need to correct for failing tests

@ThomasHaine
Copy link
Collaborator

What's the status of this PR? Can we fix and merge soon? I'm posting to the Poseidon website about ECCO functionality in OceanSpy+SciServer.

Copy link
Collaborator

@ThomasHaine ThomasHaine left a comment

Choose a reason for hiding this comment

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

Looks good to me! We should add an ECCO tutorial so folks can get going with these data quickly.

@Mikejmnez
Copy link
Collaborator

@asiddi24 The error looks pretty easy to fix. It looks like it has to do with an incompatibility between the new line

ds = ds.drop_vars(["k_p1", "k_u", "k_l", "k"])

when creating the test ospy dataset, and the data (along with .yaml file) used for testing. The error you are getting is

ValueError: One or more of the specified variables cannot be found in this dataset

(you can see this in lines 11681-11689 of the CI/Build). I think the easier fix would be to update the data for testing (I used ECCO_v4/THETA/THETA_1992_01.nc, .../UVEL_1992_01.nc, .../VVEL_1992_01.nc and the ECCO_grid.nc files) along with the catalog_ECCO.yaml to read them, in a way that is compatible with the format of the ECCO dataset that will be used on Sciserver...

@ThomasHaine
Copy link
Collaborator

@asiddi24 Is this PR ready for merge? Once the failing tests are fixed (which seems to be a universal issue with all PRs at the moment)?

@asiddi24
Copy link
Collaborator Author

tests are failing, but for now I'm going to go ahead and merge. This is so that we can test the new oceanography image.

@asiddi24 asiddi24 merged commit 8561d37 into hainegroup:master Feb 14, 2022
malmans2 added a commit that referenced this pull request Feb 14, 2022
@malmans2
Copy link
Contributor

@asiddi24 don't delete this branch until we decide what to do with #217

@asiddi24
Copy link
Collaborator Author

got it

Mikejmnez pushed a commit that referenced this pull request Feb 14, 2022
* Revert "loading ECCO v4r4 in zarr format  (#200)"

This reverts commit 8561d37.

* Update catalog_xarray.yaml

changes to catalog

* Update datasets_list.yaml

update ECCO_v4r4

* Update open_oceandataset.py

commented out the dropping of the k variable name

* Update datasets_list.yaml

reverting back to ECCO

* Update open_oceandataset.py

fixed the white space

Co-authored-by: Ali Hasan Siddiqui <43628137+asiddi24@users.noreply.github.com>
@asiddi24 asiddi24 deleted the ecco_intake branch September 13, 2022 16:13
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.

5 participants