-
Notifications
You must be signed in to change notification settings - Fork 32
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
plot arrays across faces without cutout #427
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #427 +/- ##
==========================================
- Coverage 96.59% 95.62% -0.97%
==========================================
Files 9 9
Lines 4987 5163 +176
Branches 1190 1246 +56
==========================================
+ Hits 4817 4937 +120
- Misses 97 133 +36
- Partials 73 93 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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'm trying to run your example notebook. I need catalog_ECCO.yaml
. Please can you provide it? Thanks!
Hey Tom. |
@ThomasHaine I changed a bit the notebook in the hyper link above to run in Sciserver, it also has description on how to install the correct oceanspy version (this branch) in a sciserver environment, and also added improved description of the notebook. There are about 20 examples. The notebook successfully ran in Sciserver with ECCO. I'd expect the same with LLC4320. |
@Mikejmnez thanks. I'll check... |
With LLC4320 it errors at the first plot with:
|
@Mikejmnez any ideas about how to fix?
|
Hey @ThomasHaine! Apologies, I thought I wrote but my message stayed as draft. ds = ds.set_coords(['XC', 'YC']) that does it. Otherwise However, last week as I ran this code with LLC4320 on SciServer (Oceanography (Integrated Viewer)) and found that:
I need to spend some time on it. I am not too concern about 2), but 1) is a bummer (that is slow with LLC4320). Again, perhaps with Grendel things will run much much faster. Right now I am using the basic Oceanography (Integrated Viewer) volume on Sciserver (so pretty basic). |
Thanks @Mikejmnez. I find the same issues (1) and (2). Grendel is still available, so it may be possible to test now. Otherwise, the data transfer to the new ceph cluster will happen soon (I hope). Is it possible to build the |
Not sure, but if it is that would significantly speed up the plotting in this PR, |
Fixed this problem with 2) above. It was minor issue and now plotting works as expected with LLC4320, DYAMOND and all other LLC-geometries. You can find some examples with LLC4320 (and how long it took to run) on this notebook. note
TODO: Still need to write tests and documentation ( |
This PR is ready for merger! |
Thanks @Mikejmnez . I'll take a look and review, then you can merge. |
Great. I updated the example notebook at the top of this PR conversation so you can see examples with LLC4320. You can see the timings there when the grid dataset is stored in zarr in my machine, next to the notebook. The |
@ThomasHaine BTW you should re-install NOTE: The notebook uses a cell [4]: ECCOod = ospy.open_oceandataset.from_catalog("ECCO") and cell[6]: LLC4320od = ospy.open_oceandataset.from_catalog("LLC4320") |
@Mikejmnez I checked the notebook with |
This issue closes #410 and #409
The following are tasks that need to be completed before merging this PR. Until then, consider this as a work in progress
I will work on completing the rest of the tasks sometime this week, or the weekend at the latest.