Accessors and grouping documentation entries

[zerothi]

This is an extended try of @pfebrer try in #839.

I took the details there, and played with it, and extended it a bit more.

So now basically all is done in the template files.
It ain't pretty, but is self-contained.

I kind-of-don't like it. But it seems much simpler than hacking in the python code, and extending what autodoc does.

The advantages are:

• we have manual control of how the layout should be
• we can also double document a few details, if needed, e.g. the to/plot could both be in their designated sections, but also in the Methods section.
• it is very easy to add another section
• right now everything is automatic, I just check whether an attribute of a class is an instance of a sisl._dispatch.AbstractDispatcher, and patches the attributes based on that.

Disadvantages:

• we don't have access to the actual documentation, so can't really do anything there in the jinja templates.
• the way we get the documentation is very hacky.... It overwrites attributes in the classes, which it shouldn't do, but
  it gives the effect. It just means that tool-tips might not work... :(
• fine-grained control is limited to a rather horrible jinja templating engine, which is extremely verbose, and sometimes limited.

@pfebrer let me know what you think?

Of course, the actual docs are still missing... ;)

Here are a few pics:

Type 1:

Type 2:

Type 3:

• Closes Improving documentation of plot handlers #839
• Added tests for new/changed functions?
• Ran isort . and black . [24.2.0] at top-level
• Documentation for functionality in docs/
• Changes documented in CHANGELOG.md

[codecov]

Codecov Report

Attention: Patch coverage is 74.57627% with 15 lines in your changes missing coverage. Please review.

Project coverage is 87.04%. Comparing base (08a401e) to head (1ab7cc6).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sisl/_dispatcher.py	25.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #845      +/-   ##
==========================================
- Coverage   87.06%   87.04%   -0.03%     
==========================================
  Files         402      402              
  Lines       51887    51899      +12     
==========================================
  Hits        45175    45175              
- Misses       6712     6724      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pfebrer]

Wow, first of all let me say that what you have managed to do inside the jinja template is very impressive 😅

Also, I love the automatic finding of the dispatchers to ensure everything is automatically documented. This is something that I would do, but I didn't think you would like :)

I have two suggestions:

1. Out of your disadvantages, the one that I find most important is that putting all the logic in a jinja template looks horrible and makes the template hard to edit with confidence for future developers that are not you or me. I have seen that there is a variable that you can define in conf.py, autosummary_context, that is a dictionary of variables that become available to the template (documented here, and is used like this). This is not per-class, it is a global context. But still, while we assign the nested attributes in conf.py we could build a table that is available in the template, where you would just query the table using the name of the class. This would move most of the logic out of the jinja template, and the template could be made fairly simple again.
2. We really need to find a way to show apply.iterate instead of BrillouinZone.apply.iterate. Things would look much cleaner and would be easier to interpret for the user. There is no configuration available to do this, see here how autosummary determines the display name. So I see only two options: either we wrap the get_items() method of Autosummary to postprocess the display names at our will, or we don't use the autosummary directive and build the table ourselves instead. I think that although the first option is a bit hacky it is probably the best (we are already in a hacky mood, so who cares about one more little hack 😅 )

[pfebrer]

For suggestion (2) we could also contribute to sphinx.ext.autosummary to provide a config to modify the display name. I don't know how easy/hard it is to put things in there.

[pfebrer]

Well actually we could also contribute the thing about creating a per-class context for the template.

[pfebrer]

I decided to take a chance sphinx-doc/sphinx#13003, let's see :)

[zerothi]

Wow, first of all let me say that what you have managed to do inside the jinja template is very impressive 😅

Also, I love the automatic finding of the dispatchers to ensure everything is automatically documented. This is something that I would do, but I didn't think you would like :)

I have two suggestions:

1. Out of your disadvantages, the one that I find most important is that putting all the logic in a jinja template looks horrible and makes the template hard to edit with confidence for future developers that are not you or me. I have seen that there is a variable that you can define in `conf.py`, `autosummary_context`, that is a dictionary of variables that become available to the template (documented [here](https://www.sphinx-doc.org/en/master/usage/extensions/autosummary.html#confval-autosummary_context), and is used [like this](https://github.com/sphinx-doc/sphinx/blob/2f1d775dfda9e4f81dfff6cfbe9edf7731e32a97/sphinx/ext/autosummary/generate.py#L295)). This is not per-class, it is a global context. But still, while we assign the nested attributes in `conf.py` we could build a table that is available in the template, where you would just query the table using the name of the class. This would move most of the logic out of the jinja template, and the template could be made fairly simple again.

Great, I didn't find this doc, only html_context which isn't passed to the autosummary templates, thanks, I'll have a stab at that!

2. We really need to find a way to show `apply.iterate` instead of `BrillouinZone.apply.iterate`. Things would look much cleaner and would be easier to interpret for the user. There is no configuration available to do this, see [here](https://github.com/sphinx-doc/sphinx/blob/2f1d775dfda9e4f81dfff6cfbe9edf7731e32a97/sphinx/ext/autosummary/__init__.py#L320-L323) how autosummary determines the display name. So I see only two options: either we wrap the `get_items()` method of `Autosummary` to postprocess the display names at our will, or we don't use the autosummary directive and build the table ourselves instead. I think that although the first option is a bit hacky it is probably the best (we are already in a hacky mood, so who cares about one more little hack 😅 )

Yes, that is really required, but so far still manageable if BrillouinZone is shown (not pretty... But...).

[zerothi]

I can't figure out how to do namespace retrieval of variables...

{% set check = namespace() %}
{% for item in ["foo", "bar"] %}
{% set check.{{item}} = false %} <- does not work
{% set check[{{item}}] = false %} <- does not work
{% endfor %}

so, some variables just needs to be hard-coded... :(

[pfebrer]

It should be check[item], no?

[pfebrer]

And probably without the set:

check[item] = false

But I don't understand why you need to set things inside the template.

[zerothi]

I tried both, none work...

The idea was to have the special fields defined in the conf.py, and then dynamically create the namespace to check if the section should be there or not. I can't find a way around this... :(

[pfebrer]

Why do you use namespace? Can't you create a normal dictionary?

[pfebrer]

But I don't understand why you need to set variables inside the template, can't you set them all in conf.py?

[zerothi]

No, because I want to change them in the template. I have to discover the methods in the template. I.e. if plot.* exists, then create Plotting section, else don't. And I can't figure out a way to do this programmatically.

I found a way, it ain't pretty.. Dynamic lists are the way to go...

[pfebrer]

What I had in mind was that in conf.py you could build a context like:

autosummary_context = {
   "info": {
       "Hamiltonian": {...info for the Hamiltonian},
       "BrillouinZone": {...info for the BrillouinZone}
    }
}

And then in the template do something like:

class_info = info[objname]

So you would compute all the needed variables for each class in conf.py

[zerothi]

What I had in mind was that in conf.py you could build a context like:

autosummary_context = {
   "info": {
       "Hamiltonian": {...info for the Hamiltonian},
       "BrillouinZone": {...info for the BrillouinZone}
    }
}

And then in the template do something like:

class_info = info[objname]

So you would compute all the needed variables for each class in conf.py

Lets, take that as a 2nd step, lets start by getting this to work, which is already hard enough ;)

Also, parsing dictionaries in jinja is not exactly fun.... ;)

[pfebrer]

Ok, I thought it would be much easier. I don't understand what you want to use the autosummary_context for then, but let's see.

[zerothi]

@pfebrer I think this is ready for a quick review, just whether you like the overall details?

Some issues remaining:

The method that is documented is controlled here
As you can see, one can finetune these details.
You can try and building this in two cases, one as it is, and one where it has:

        dispatcher_name = (dispatcher_name, None)

then you'll find:

1. building it with no changes uses the docstring from the dispatch methods, which actually does the lifting. So here you get the parameter list from that dispatch method, which isn't correct..
2. building it with the change uses the docstring for the class that holds the dispatch method. Here you get the __call__ method documented, which isn't great either.

In both cases are the signatures wrong. And generally the signatures change depending on what you request provide.
So how to solve this is not exactly clear to me...

You'll also note that the latest commits fixes #728 ! :)

• I tr

[pfebrer]

I think signatures should display exactly the way in which the functions can be called. This is what I will do with the plotting in #839 once this gets in.

I guess logically what should be documented is the __call__ method, because this is also what code editors (e.g. jupyter notebook) will show. The approach I follow with the plot handlers is this:

sisl/src/sisl/viz/_plotables.py

Lines 80 to 88 in 0af33db

	return type(
	f"Plot{name.capitalize()}Dispatch",
	(PlotDispatch,),
	{
	"_plot": staticmethod(function),
	"__doc__": function.__doc__,
	"__signature__": inspect.signature(function),
	},
	)

That is, I create a subclass for each dispatch to be able to document them individually.

[pfebrer]

Are you sure that you want to display apply dispatchs as functions? I think across sisl documentation the displayed usage is as an attribute:

bz.apply.average.eigenstate()

instead of:

bz.apply.average("eigenstate")

Isn't it?

[pfebrer]

While we can not modify upstream autosummary, here is a workaround for display names:

EDIT: I think the approach in the next comment is better.

This will use "|" as a marker for where the display name starts, as you were proposing:

from sphinx.ext.autosummary import Autosummary

class CustomAutosummary(Autosummary):
    """Wrapper around the autosummary directive to allow for custom display names.
    
    In particular, | is interpreted as the mark to indicate where the display name
    should begin.
    """

    def get_items(self, names):

        # Find all names that have a | marker, and store their index along with the name that we want to display
        display_names = [(i, name.split("|")[-1]) for i, name in enumerate(names) if "|" in name ]
        # Clean the names so that Autosummary can process them
        names = [name.replace("|", "") for name in names]

        # Process names with Autosummary
        items = super().get_items(names)

        # Substitute the display names that we determined. This is only possible if the number of
        # items is the same as the number of names, i.e. no import errors have ocurred for any item.
        # Otherwise the indices will not match.
        if len(names) == len(items):
            for i, name in display_names:
                items[i] = (name, *items[i][1:])

        return items

This custom class must be registered as the autosummary directive:

def setup(app):
    app.add_directive("autosummary", CustomAutosummary, override=True)

Then you add the "|" marks to the template and it looks beautiful :)

[pfebrer]

Another approach to display names that I think is much more robust:

It adds an option :removeprefix: that you can use to remove a certain prefix from the display name.

from sphinx.ext.autosummary import Autosummary
from docutils.parsers.rst import directives

class CustomAutosummary(Autosummary):
    """Wrapper around the autosummary directive to allow for custom display names.
    
    Adds a new option `:removeprefix:` which removes a prefix from the display names.
    """

    option_spec = {
        **Autosummary.option_spec,
        "removeprefix": directives.unchanged
    }

    def get_items(self, *args, **kwargs):
        items = super().get_items(*args, **kwargs)

        remove_prefix = self.options.get("removeprefix")
        if remove_prefix is not None:
            items = [(item[0].removeprefix(remove_prefix), *item[1:]) for item in items]

        return items

def setup(app):
    app.add_directive("autosummary", CustomAutosummary, override=True)

Then you add the option to the autosummary directives for the dispatchers, e.g.:

.. autosummary::
    :removeprefix: {{ name }}.

    {{ name }}.plot
    {{ extract_startswith('plot.', false, attributes, methods) }}

[zerothi]

@pfebrer thanks, I have implemented your last suggestion!!!

[zerothi]

@pfebrer perhaps we should merge this, and then continue on the documentation stuff in later PR's?

[pfebrer]

Yes, sounds good! It is already much better than it was 👍

Now it is just a matter of making sure all the dispatchs are properly documented in general, and sphinx is ready to show the documentation.

[zerothi]

This superseeds #839.