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

Add woa23 #21

Merged
merged 8 commits into from
May 16, 2024
Merged

Add woa23 #21

merged 8 commits into from
May 16, 2024

Conversation

gaelforget
Copy link
Contributor

@gaelforget gaelforget commented May 15, 2024

PR:

CI:

  • all active tests pass on my laptop but see GitHub actions dont trigger upon PR #23
  • I also ran the inactive test/test_urls.jl but some fail (as expected)
  • some of the old locations for WOA09 and 13 seem deprecated
  • I also build the docs manually (no issue)

@@ -55,9 +55,9 @@ function mean_std_and_number_obs(ds, tracer)
lon = mod.(ds["lon"][:] .|> Float64, 360)
lat = ds["lat"][:] .|> Float64
depth = ds["depth"][:] .|> Float64
μ3D = ds[WOA_varname(tracer, "mn")][:][:, :, :, 1]
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind explaining this change? If I remember correctly this X[:][:,:,:,1] syntax was required for the array to be materialized (and not just a lazy representation) and this made the processing that followed faster. Maybe I'm remembering incorrectly or things have changed upstream since then?

Copy link
Contributor Author

@gaelforget gaelforget May 16, 2024

Choose a reason for hiding this comment

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

I believe this is needed with NCDatasets.jl 0.14 (compat was limited to 0.12 in Project.toml) where X[:] results in a 1D vector. Maybe the compat entry should be NCDatasets = "0.14" with this change. Not sure but seems better than sticking with 0.12.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes that's better I'll do that

@briochemc
Copy link
Owner

briochemc commented May 16, 2024

Thank you very much for this PR! Looks good to me and happy to merge. I'll just wait for your reply to my review comment above! 🙂

@briochemc briochemc merged commit 2d29462 into briochemc:master May 16, 2024
7 checks passed
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