Add note about get_video convention to ImagingExtractor

[h-mayorquin]

Related to #156

The ideal will be to add this in a section of the read the docs but we are far from it. The standard should be written somewhere and this is a place.

[pauladkisson]

In general, get_video only returns 3D numpy ndarrays (frames, height, width). Depth is handled only for some extractors, so they should override their get_video docstring to specify how it is treated.

[h-mayorquin]

Every method needs to override the whole function and therefore also the docstring as this is an abstract method. In other words, this is not user facing documentation misguiding the users but documentation for developers with instructions of a common pitfall when they overide the method. In that sense, I think it is better served if it has the more general case, more information is better than less in this case.

[pauladkisson]

Every method needs to override the whole function and therefore also the docstring as this is an abstract method.

Most of the current ImagingExtractors return video exactly as it's described in the base class, so they can inherit the docstring without needing to rewrite it. I believe that is one of the pros of defining an abstract get_video function.

In that sense, I think it is better served if it has the more general case, more information is better than less in this case.

More information can be helpful for devs, but let's at least mention that the return video can be 3D or 4D depending on whether the extractor defines depth.

[h-mayorquin]

Every method needs to override the whole function and therefore also the docstring as this is an abstract method.

Most of the current ImagingExtractors return video exactly as it's described in the base class, so they can inherit the docstring without needing to rewrite it. I believe that is one of the pros of defining an abstract get_video function.

Maybe you have a plan that I don't understand. How are you going to inherit the docstring of the abstract method?

This seems like a non-standard trick, is that something that you guys have discussed, @CodyCBakerPhD ?

[pauladkisson]

How are you going to inherit the docstring of the abstract method?

The child class inherits the docstrings of the parent methods automatically even if it overrides the body of the method -- as long as it doesn't define its own docstring.

[h-mayorquin]

It does not seem that this is true to me, can you reference this or show me this behavior? If it is, what's missing in the code below?

from abc import ABC, abstractmethod

class AbstractClass(ABC):
    """
    This is the AbstractClass's docstring.
    """
    
    @abstractmethod
    def abstract_method(self):
        """
        This is the abstract_method's docstring.
        """
        pass
    
    @abstract_method
    def another_abstract_method(self):
        """
        This is the another_abstract_method's docstring.
        """
        pass
    
    def a_normal_method(self):
        """Parent docstring"""

class ChildClass(AbstractClass):
    
    def abstract_method(self):
        return "Implemented in ChildClass"

    def another_abstract_method(self):
        """This one has a docstring"""
        return "Implemented in ChildClass"
    
    def a_normal_method(self):
        return "Do I override the parent's docstring?"
# Test
child = ChildClass()
print(child.abstract_method())
print(child.abstract_method.__doc__)
print(child.another_abstract_method.__doc__)
print(child.a_normal_method())
print(child.a_normal_method.__doc__)```

Output:

Implemented in ChildClass
None
This one has a docstring
Do I override the parent's docstring?
None

[h-mayorquin]

It would be nice to get a response for this, @pauladkisson. I think that you are wrong in your claim above.

[CodyCBakerPhD]

@h-mayorquin Note that directly accessing the __doc__ is not how we generally instruct users to view docstrings on instances of objects

Check out our recommended ways

child.abstract_method?
help(child.abstract_method)

This discrepancy is still good for all of us to take note of

[h-mayorquin]

That seems convicing to me:

I will implement the requested changes.

[pauladkisson]

@h-mayorquin, sorry for the delayed response. Basically, as Cody pointed out, dynamic docstring look-up via help finds inherited docstrings, even though __doc__ doesn't. We are taking advantage of that functionality to only write docstrings for the base class methods (get_num_channels, get_image_size, etc.) in the base class implementation UNLESS a child class specifically changes its inputs/outputs, in which case it would need to write its own docstring for the altered method.

[h-mayorquin]

That's fine, this was useful to me. I was not aware that python performs attribute lookup even for attributes of the class' methods when using the help builtin. At the beginning I thought that this was ipython / jupyter behavior but I also tried on the python REPL and is the same behavior.

[h-mayorquin]

Changed this.

[pauladkisson]

One formatting change otherwise LGTM