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

Improving documentation of plot handlers #839

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Oct 5, 2024

This is related to #838

Most changes are just general improvements of the automatic documentation of plot handlers (regardless of who renders it).

The changes related to sphinx are only in docs/conf.py and docs/build_docs.sh, where I followed the approach explained in #838 (comment). I also made conf.py document the Geometry.to and BrillouinZone.apply dispatchers, as the difficulty to document dispatchers also affects them.

I have tried to use sphinx events, but I haven't succeeded. If you come up with a better way to document nested methods inside dispatchers then great, but I think that the current method, although hacky, it produces nice documentation results:

Plotting methods in the summary:
Screenshot from 2024-10-05 18-17-52

Plotting methods documentation:
Screenshot from 2024-10-05 18-18-11

Geometry.to dispatchs:
Screenshot from 2024-10-05 18-18-43

(this PR is not ready yet because I want to further improve the docstrings of the plotting methods, it's just so that you can see the approach that I'm taking and provide better ideas)

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 5, 2024

In this PR the automatic docstrings for plot handlers depend on numpydoc(https://github.com/numpy/numpydoc), which I think is not super bad to add as a dependency to sisl.viz as it avoids plenty of boilerplate code and it is a lightweight pure python package.

@zerothi
Copy link
Owner

zerothi commented Oct 7, 2024

Hmm.. I have looked a bit into this.

And it seems this is the wrong way to go about this.

  • xarray
  • pandas
  • etc.

are all explicitly creating their api.rst documents. Perhaps we should too?

This would also enable accessor documentation as we would like it:

Plotting
--------

.. autosummary::
   :toctree: generated/
   :template: autosummary/accessor_method.rst

   Dataset.plot.scatter
   Dataset.plot.quiver
   Dataset.plot.streamplot

above is an excerpt from xarray.

Only thing that is needed is the package sphinx-autosummary-accessors.

I think this could solve all issues, albeit it would be a somewhat hurdle to get right for the first time and it requires us to manually add functions to the documentation. However, perhaps that could solve other issues, such as grouping, proper documentation style etc.

What would be your thoughts on this transition?

@zerothi
Copy link
Owner

zerothi commented Oct 7, 2024

It might require a bit of adjustment and play with it. But it seems to be easier to manage in the long run, and I don't like the hacky way of building a project with errors, then patching it and hoping nothing broke... It is too fragile IMHO.

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 7, 2024

I think it could make sense to create our custom .rst files but doing it manually seems like a very bad idea, because it is just a matter of time that something will get outdated.

We could have some scripts to automatically generate the rst files.

and I don't like the hacky way of building a project with errors, then patching it and hoping nothing broke... It is too fragile IMHO.

Users are never going to build the documentation, it is always built in a controlled environment, so it is not so clear to me that the approach is that bad. But yeah if you have an idea for automatizing the custom rst files it is better of course. I can do it if you have a clear picture of what you want.

Otherwise if this issue is going to stall because of lack of time I think having the documentation built with the hack is much better than having undocumented parts of the code.

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 7, 2024

The replacement of the placeholder could be done also within sphinx I think, but I haven't found the exact way to do it yet. That would be less hacky in the sense that it doesn't need external scripts so it would work every time

@zerothi
Copy link
Owner

zerothi commented Oct 7, 2024

I think it could make sense to create our custom .rst files but doing it manually seems like a very bad idea, because it is just a matter of time that something will get outdated.

It is only manual for the overview, not for the actual documentation. I.e. see here.
It is definitely not everything, but I think it is doable.

We could have some scripts to automatically generate the rst files.
Sure, that could work, but to allow more complicated things, perhaps this is fine.

and I don't like the hacky way of building a project with errors, then patching it and hoping nothing broke... It is too fragile IMHO.

Users are never going to build the documentation, it is always built in a controlled environment, so it is not so clear to me that the approach is that bad. But yeah if you have an idea for automatizing the custom rst files it is better of course. I can do it if you have a clear picture of what you want.

Just like we had an issue with the notebooks not fully working (which required us to actually see it) this could pose problems. I agree users don't interwene, but these things always bite back at some point...

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 7, 2024

It is only manual for the overview, not for the actual documentation. I.e. see here.
It is definitely not everything, but I think it is doable.

That file is generated automatically, no?

@zerothi
Copy link
Owner

zerothi commented Oct 7, 2024

It is only manual for the overview, not for the actual documentation. I.e. see here.
It is definitely not everything, but I think it is doable.

That file is generated automatically, no?

I am pretty sure its hand-made... But rather simple, right, the Dataset methods/attributes are not here.
The way xarray does this is to not document the plot method, that one is documented here so they are kind of separating these things. Annoying, but perhaps makes sense, since it

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 7, 2024

Actually I find xarray's documentation hard to navigate. Each method has its own page and therefore to see the documentation for another method of the same class you have to go back to the main page, look at the list of methods and click on the other one that you think might be what you want. The navigation that they provide at the bottom saying "Previous", "Next" is useless in my opinion:

Screenshot from 2024-10-07 14-15-13

In any case, I think xarray is quite different because it mainly has two important classes: DataArray and Dataset. The rest of classes are very rarely used. In sisl there are lots of classes that are important and I think it is a good idea to keep the methods for each class contained on their own page.

I have been playing with sphinx-autosummary-accessors and I don't get what I want because it only creates a new page for the accessor method, it doesn't add the method documentation's in the page of the class as all other methods. Also I haven't managed to make it document the signature and the help message for each parameter, I just got this:

Screenshot from 2024-10-07 14-22-43

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