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

Qiskit SDK: revisit inheritance, including autoclass missing inheritance #2376

Closed
Eric-Arellano opened this issue Nov 22, 2024 · 3 comments
Closed

Comments

@Eric-Arellano
Copy link
Collaborator

We are doing two unexpected things with how we show inheritance in Qiskit SDK docs:

  1. When using autosummary, we have a template called class_no_inherited_members.rst. It successfully leaves off inherited methods, but it shows inherited attributes! I think this is an oversight. To fix, we can update the template to use these lines:

https://github.com/Qiskit/qiskit-addon-mpf/blob/191dc8162510719ca3b50e5a25e9a1453419629a/docs/_templates/autosummary/class_without_inherited_members.rst?plain=1#L16-L24

However, that means that some of the classes in the circuit library will end up nearly empty. Is this what we want? One approach we could take is adding a warning like this:

https://github.com/Qiskit/qiskit-addon-mpf/blob/191dc8162510719ca3b50e5a25e9a1453419629a/qiskit_addon_mpf/backends/quimb_tebd/evolver.py#L34-L37

  1. Show inherited members by default in .. autoclass:: for inline classes? By default, we show inherited members for classes with .. autosummary::, but we don't do that for .. autoclass::. I think the default of inherited members is a sensible default because users don't care about where a method/attribute comes from, which is an implementation detail. It's a gotcha that you have to remember in .. autoclass:: to include inherited members. We can fix this by setting in conf.py:
autodoc_default_options = {
    "inherited-members": None,
}

To fix this, we need sign off from Qiskit devs then to get the implementation green. I had a lot of issues with cross-references breaking when turning it on.

@Eric-Arellano
Copy link
Collaborator Author

Update from meeting with Qiskit devs:

Question 1 about autosummary:

  • update every class to show inherited members, except for QuantumCircuit subclasses like QuantumVolume. This means use the default template
  • update no members template to not show inherited attributes
  • anything still using the no inherited members should have a note admonition about inherited class, like the MPF addon

Question 2 about autoclass:

  • probably keep it the same because Jake Lishman is the main person to use .. autoclass:: and he is particular about its usage.
  • check in with Jake when back from leave

@jakelishman
Copy link
Member

about point 2: lol, and also, I'm fine with whatever you prefer the default to be, since I can override it when I'm writing very custom documentation where it's (imo) better one way or the other.

@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented Nov 26, 2024

about point 2: lol

Hahah no one wanted to make a decision without your input

I'm fine with whatever you prefer the default to be, since I can override it when I'm writing very custom documentation where it's (imo) better one way or the other.

Cool. I recommend setting it to show inherited by default. We all agreed in the meeting that usually that's the better approach: users *usually don't care whether something is inherited or not. Do the safer thing by default.

--

I've opened #2397 and #2400 to track these follow ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants