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

Remove properties that aren't implemented from docs #184

Open
tcmoore3 opened this issue May 19, 2023 · 3 comments
Open

Remove properties that aren't implemented from docs #184

tcmoore3 opened this issue May 19, 2023 · 3 comments
Assignees

Comments

@tcmoore3
Copy link
Member

Feature description

It should be clear in the documentation which properties are not implemented for a given shape. For example, 'minimal_centered_bounding_sphere' is not implemented for shapes.ConvexSpheropolyhedron but this is not clear in the documentation.

Proposed solution

Either remove those properties from the class documentation or somehow make it clear that they are not implemented.

@tcmoore3 tcmoore3 changed the title Remove att that aren't implemented from docs Remove propertie that aren't implemented from docs May 19, 2023
@tcmoore3 tcmoore3 changed the title Remove propertie that aren't implemented from docs Remove properties that aren't implemented from docs May 19, 2023
@janbridley janbridley self-assigned this Jun 19, 2023
@janbridley
Copy link
Collaborator

ConvexSpheropolyhedron inherits Shape3D which includes dummy methods for several properties that are not implemented in the base class. The documentation is built automatically with readthedocs automodule and the inheritance flag adds these dummy methods to the documentation. This is also an issue for ConvexSpheropolygon and a few other shapes.

The cleanest way to solve this would be to selectively disable :show-inheritance: for the desired classes - however, this is nontrivial as far as I can tell. The best alternative I see is to manually include an :autoclass: for each class in the module with different inheritance settings for each. This is not a perfect fix though, and I would be curious if @vyasr has any input on either solution?

@vyasr
Copy link
Contributor

vyasr commented Jun 21, 2023

One alternative would be to simply remove the base class implementations.

Having these "abstract" (in quotes because they aren't really abstract in the sense of Python's abc module) methods and properties implemented on the parent class serves two purposes:

  1. Conceptual: It provides the standard definition of a particular property. The very long, descriptive property names are meant to avoid ambiguity because terms like "insphere" are very imprecisely used in a lot of our research.
  2. Practical: It allows sharing docstrings across subclasses. Sphinx supports docstring inheritance, so subclasses can implement these abstract methods but leave the docstrings blank and just inherit them from the parent class.

The benefits with respect to item 1 are a bit questionable. Since coxeter doesn't use true ABC abstractmethods (because subclasses don't have to implement a particular property) the current approach also doesn't really enforce that subclasses use a particular name for an attribute. A subclass FancyShape could always use some arbitrary definition of e.g. FancyShape.insphere. It's up to developers to ensure that a property name is used consistently between different shape classes. From that perspective, removing a "standard" implementation from the parent class doesn't really lose us much in terms of consistency.

For item 2, we could recoup most of the same benefits by using a simple decorator that modifies the method's doc attribute, like so:

def set_docstring(doc):
    def wrapper(func):
        func.__doc__ = doc
        return func
    return wrapper

from .docstrings import minimal_centered_bounding_sphere_doc

class FancyShape(Shape):
    @property
    @set_docstring(minimal_centered_bounding_sphere_doc):
    def minimal_centered_bounding_sphere(self):
        ...

@janbridley
Copy link
Collaborator

janbridley commented Jun 22, 2023

Noted, I will give that a try

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

No branches or pull requests

3 participants