Skip to content

Conversation

@kgoebber
Copy link
Collaborator

@kgoebber kgoebber commented Mar 9, 2023

Description Of Changes

This is a first pass at updating the ways that the areas baked into the declarative syntax works. I've captured 331 areas (a subset of what was in our original list) and have them corresponding to both an extent and a Cartopy projection. This is all contained in a separate file now and removed from the declarative file. These areas, their extents, and the projections are also available outside of the declarative syntax as well, which is captured the tutorial.

In adding elements to the docs I've attempted to capture things in a tutorial as well as a separate listing of all of the areas with a graphical representation of those areas with their long descriptive name. This generates 331 images, so the build of the docs takes longer, but if the file is not changed, then the caching of the images works as intended.

Still to do is to add tests.

Open to suggestions about organization of any of this material.

Checklist

@kgoebber kgoebber force-pushed the update_areas branch 2 times, most recently from d658557 to 70f7473 Compare March 16, 2023 20:15
@kgoebber kgoebber force-pushed the update_areas branch 9 times, most recently from 8f478c2 to f5fe981 Compare March 18, 2023 20:14
@kgoebber kgoebber marked this pull request as ready for review March 18, 2023 21:07
@kgoebber kgoebber requested a review from a team as a code owner March 18, 2023 21:07
@kgoebber kgoebber requested review from dcamron and removed request for a team March 18, 2023 21:07
@dopplershift
Copy link
Member

  1. Any chance area_data was generated so that you could change it easily? I have some performance concerns about creating that many tiny dictionaries at import-time, only to throw them away (vs. e.g. named tuples).
  2. I'm not wild about checking in a generated file. Do we really need it committed?

@kgoebber
Copy link
Collaborator Author

  1. It was partially generated - manually did some line wrapping because of character limit, but if I take the time I should be able to do that as well. I can look at using a named tuple instead - initially went with what I knew (e.g., dictionaries) - but I'm not wedded to that if there would be better performance/better data object to use.
  2. I tried to have the area.rst generate during the build, but I couldn't seem to find the right incantation of things through all of the sphinx docs settings, etc without a lot of excess stuff being generated as we have set up for a tutorial or gallery page. Reading of the Sphinx documentation was not yielding any clear paths to me from what I was able to find. In the end, I took the inspiration from Cartopy for the generation of their projections list. I would love to have that auto generate instead of committing a generated file.

I'll start looking at a named tuple for creating the areas and if there are any resources you want to point me toward for being able to auto generate the documentation file, I would be very happy to entertain.

Any other thoughts about functionality or organization of the information/documentation?

@kgoebber kgoebber force-pushed the update_areas branch 2 times, most recently from 2ffba12 to 7248c6d Compare April 6, 2023 19:49
@kgoebber
Copy link
Collaborator Author

kgoebber commented Apr 7, 2023

Okay, so I've updated this PR with what ended up being a very simple way of generating the area.rst file to both make files. I removed the committed area.rst and we should be relying on the autogenerated one from the Python file.

@dcamron
Copy link
Member

dcamron commented Apr 11, 2023

Would you mind test out using def setup(app) in conf.py, basically a built-in Sphinx Extension to run your area generation script? It should work to run your Python code before building the docs whenever we call sphinx build.

If you find yourself needing to hand this off, I can dig into that, but it could let us keep your script as part of our sphinx tooling and out of the separate make machinery.

@kgoebber
Copy link
Collaborator Author

Okay, couldn't find a way with a more specific sphinx way of creating the areas.rst file, but I have been able to bake in a method for running the make_areas.py file within conf.py, which seems more robust than adding it to Makefile and make.bat.

@dopplershift
Copy link
Member

I've got the best path to integrate with Sphinx here figured out. I'll push some changes to this PR.

@dopplershift
Copy link
Member

So this isn't much different than what was done (running inside conf.py), but we can list pretty much any Python file in the same directory as conf.py as an extension. That extension then has setup(), which registers a callback for an event, which in our case we wait for the "builder" to be initialized.

Other than that it's the same code, though it relies on Sphinx to know some proper paths and give us a logger. Sphinx doesn't have much better in the way of hooks to grab, but this does seem a bit more structured than running a script every time conf.py is imported. It's actually the same way Sphinx's own autosummary extension is wired in.

@dopplershift dopplershift added the Type: Enhancement Enhancement to existing functionality label Apr 26, 2023
@dopplershift dopplershift added this to the April 2023 milestone Apr 26, 2023
@dopplershift
Copy link
Member

@dcamron This will need your review since I've now committed on the PR.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Sorry, finally noticed a few things here.

If you can't get to them before (trying to) release tomorrow, I'll take care of them.

@kgoebber kgoebber force-pushed the update_areas branch 3 times, most recently from f4e196c to 62df8de Compare May 4, 2023 03:39
@kgoebber
Copy link
Collaborator Author

kgoebber commented May 4, 2023

I was able to make the requested changes, but don't have the correct method of exposing the named_areas from plots directly and not run afoul of import things. I think you all will have to take it from here.

kgoebber and others added 6 commits May 4, 2023 16:17
This changes things minimally, but hooks into the Sphinx build process
in a more structured way. This allows getting the build directory more
robustly and allows us to use Sphinx logging, as well as generating at a
better time than doing the build every time conf.py is imported/run.
dopplershift
dopplershift previously approved these changes May 4, 2023
Since building the full area file can take awhile with all the plots,
add a config to control it. By default, we only build the full file on
CI.
@dopplershift
Copy link
Member

I confirmed that the default to dynamically control whether we generate ALL the areas is working properly on CI vs. locally. This is a significant time savings for building locally (when you don't actually care about having them all).

Copy link
Member

@dcamron dcamron left a comment

Choose a reason for hiding this comment

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

👍 thanks y'all for getting this all there!

@dcamron dcamron merged commit ab13494 into Unidata:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Enhancement Enhancement to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Declarative Syntax Area descriptions

3 participants