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 to analysis doc #504

Closed
wants to merge 11 commits into from
Closed

add to analysis doc #504

wants to merge 11 commits into from

Conversation

miniufo
Copy link
Contributor

@miniufo miniufo commented Mar 28, 2024

I've add some contents to the analysis doc. In the last section I put all of the diagnostics into a callback, following @milankl's tips. Please check if everything looks OK.

@miniufo
Copy link
Contributor Author

miniufo commented Mar 28, 2024

Some links in the analysis doc need to be verified.

@milankl milankl added the documentation 📚 Improvements or additions to documentation label Mar 28, 2024
@milankl milankl linked an issue Mar 28, 2024 that may be closed by this pull request
@milankl
Copy link
Member

milankl commented Mar 28, 2024

@miniufo This is fantastic, thank you so much. Looks great, thank you for this contribution! I just went through it and I basically only changed, apart from small things,

  • I've let your last example directly create a figure with savefig and show this after it has been created. This way the plot always represents the current version of SpeedyWeather and updates automatically. This is useful because that way we use the examples in the documentation as a test, which can also fail if we accidentally create a breaking change in the interface for example. That's super helpful, but means that the building of the documentation should generally finish in a reasonable time, so while I understand that you prefer to do the analysis at a resolution of T170, I went back down to T31 to have it quick here.
  • In a few places you used global variables inside functions, that's absolutely reasonable when hacking along in the REPL, but for reproducibility in the documentation I've removed those. This also means that the global_diagnostics.nc is now just created in the current folder pwd() (because users may add a GlobalDiagnostics callback without setting output=true)
  • I've renamend "enstrophy" to "potential enstrophy" just to avoid confusion with relative vorticity squared, which I believe most (well at leas me 😉 ) would understand enstrophy as
  • I've added a time axis to the callback and the netCDF file too so that we can plot over time on the xaxis as well

If the documentation successfully builds then you can check what it looks like here

https://speedyweather.github.io/SpeedyWeather.jl/previews/PR504

so that you can approve before it gets merged! Can you actually see the details of why the documentation is failing? (Maybe not because you're not part of our organisation, yet?)

@milankl
Copy link
Member

milankl commented Mar 28, 2024

One error was also thrown because you wrote model.planet.radius, which I agree would be intuitive, but the radius lives in model.spectral_grid.radius, because the SpectralGrid object defines the physical domain we are simulating (a sphere of radius R) as well as its discretization (in grid-point and spectral space). Also, the radius is such a central quantity relevant to many other components that it needs to passed on with spectral_grid otherwise everything would depend on planet...! (Anyway, software engineering problems...)

@miniufo
Copy link
Contributor Author

miniufo commented Mar 29, 2024

Totally agreed with all your points. It is great that everything is automatic in the flow. But there is a two-language problem for me: I use python, so writing julia codes for a nice plot (panels, share axes etc.) is not easy for me. So the plot is generated by matplotlib which I used a lot. I just learn a lot from your suggestions (julia levels up...).

https://speedyweather.github.io/SpeedyWeather.jl/previews/PR504

I cannot see anything from the link (404 file not found). The checks are not completed (the third one) but I can't find any buttom to click:
1711676508978

@milankl
Copy link
Member

milankl commented Mar 29, 2024

That's the same for me right now, normally you can just see a preview of the new docs but for some reason it doesn't deploy. I'm on it and ping you when it is!

@milankl
Copy link
Member

milankl commented Mar 29, 2024

On plotting, yeah at some point I want to do it all with Makie but I'm also still way more used to matplotlib. Hence it's PythonPlot in the documentation for now and that's fine.

@milankl
Copy link
Member

milankl commented Mar 29, 2024

Apparently it doesn't deploy because

│ - ✘ PR originates from the same repository

@navidcy We can't merge because it isn't deploying, and it's not deploying because the PR comes from a fork. Any idea how to solve this? Shall we first merge it into a branch in this repo and then into main?

@navidcy
Copy link
Collaborator

navidcy commented Mar 29, 2024

we can merge I think -- it'll deploy then.

or we can invite @miniufo as a collaborator in the repo and do what @josuemtzmo did (which I don't know really) to transform all the commits to this PR -> CliMA/Oceananigans.jl#3506 that was coming from their fork to this PR -> CliMA/Oceananigans.jl#3512 on Oceananigans repo.

@navidcy
Copy link
Collaborator

navidcy commented Mar 29, 2024

closing in favor of #505

@navidcy navidcy closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verifying conserved quantities
3 participants