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

Revert "loading ECCO v4r4 in zarr format " #217

Merged
merged 6 commits into from
Feb 14, 2022
Merged

Conversation

malmans2
Copy link
Contributor

@malmans2 malmans2 commented Feb 14, 2022

Reverts #200

@asiddi24 #200 might need some thoughts. I'm opening a revert PR, just in case...
We can close if everything looks good.

There's a few things to check in #200:

  • It breaks different tests, so there might be something to fix in that PR
  • It breaks the documentation
  • Doesn't pass pre-commit, try to run this locally pre-commit run --all

@Mikejmnez
Copy link
Collaborator

Mikejmnez commented Feb 14, 2022

I checked quickly PE #200. Yes, indeed it is breaking some basic tests. The line that is causing some trouble in open_oceandataset is

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

It is not finding some of these variables... It could be related with the dataset that is being used for testing. For now just comment this line. There is some funny behavior here.

Like Mattia says, running

pre-commit run --all

should fix the formatting issues. The only issue is really a single whitespace in the intake catalog.

And lastly, the documentation is breaking because it is not recognizing [ECCO] in the docs/conf.py file.

Perhaps setting it back to ECCO (insterad of ECCOv4) will work?

Screen Shot 2022-02-14 at 5 01 34 PM
as a dataset.

@asiddi24
Copy link
Collaborator

Yeah, it is the line in open_oceandataset that's failing it.

Perhaps setting it back to ECCO (insterad of ECCOv4) will work?

Will do.

commented out the dropping of the k variable name
@pep8speaks
Copy link

pep8speaks commented Feb 14, 2022

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-14 22:22:53 UTC

reverting back to ECCO
fixed the white space
@asiddi24
Copy link
Collaborator

okay, looks like the checks are now being passed better than in #200. @malmans2 this checks all 3 concerns ? @Mikejmnez does it look good now ?

@malmans2
Copy link
Contributor Author

okay, looks like the checks are now being passed better than in #200. @malmans2 this checks all 3 concerns ? @Mikejmnez does it look good now ?

Yup! Wait for @Mikejmnez approval to merge, but CI looks good. (Is this superseding #200? I.e., all changes you made in #200 are now here?)

@asiddi24
Copy link
Collaborator

All changes in #200 are here.

@Mikejmnez Mikejmnez merged commit 5edc932 into master Feb 14, 2022
@Mikejmnez Mikejmnez deleted the revert-200-ecco_intake branch February 14, 2022 22:46
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.

4 participants