mne-python
mne-python copied to clipboard
mne-tools
DOC: Document inst better
In most Returns we just do instance of Raw, Epochs, or Evoked. It's not optimal and in some cases not even complete (e.g., can be AverageTFR, STC, etc. instead) so we could improve this.
Options:
- Create a new category like
MNE-objectwith a glossary entry - Say something like
instance of original typeor something more compact that has this same meaning to say "you get back the same instance mostly for chaining purposes", maybe with a glossary entry. - Figure out some magical way to put the class name in the super class doc. This isn't possible via autodoc, so we'd maybe have to do some runtime replacement. For example if we decorated each class with a
@copy_name_to_methodswe could replace every method with one that has the same signature, wraps directly to the super, but replaces__doc__with one filled with the appropriate class name. But this is a lot of work for little gain.
I actually prefer (2) to (3) even if we can figure out a way to do (3) because it makes it clear you're actually getting back the original object for chaining purposes. In cases where it is a copy / different instance, we should explicitly say so somehow probably, maybe with another name like copied instance of same type or something. But we'd need to think about how to do this in a NumpyDoc-acceptable manner.
Originally posted by @larsoner in https://github.com/mne-tools/mne-python/pull/10945#discussion_r930217055
When read the title of this issue, I was actually expecting (hoping) it to be about all the places where we emit log messages (or have parts in the documentation) that literally mention "inst", e.g. something like, "… call inst.some_method()" or so (I don't have an explicit example now)
I think these are downright confusing for "common" users, esp. if nowhere in the signature of the function that produced this message an inst parameter exists.
But yeah … that's another issue, it seems
👍 for (2). There is a new Self type in typing, but let's better not go down this route.
I have to say I never understood why we are explicitly prepending "instance of". It's inconsistent with how we specify input types, where we usually don't have these two words.
I have to say I never understood why we are explicitly prepending "instance of". It's inconsistent with how we specify input types, where we usually don't have these two words.
Absolutely agree! Especially in Python, where everything is an object (aka instance). I'd remove this.
Absolutely agree! Especially in Python, where everything is an object (aka instance). I'd remove this.
I assume the thinking was something like, "If the returned object is of type mne.Epochs, what we actually return is not a pointer to the class mne.Epochs, but the result of mne.Epochs(...), i.e., an instance". But … this is so trivial that it's downright confusing to make it explicit, because it now requires thinking where usually one doesn't need to think, at least not if you're a user 😀
I actually prefer (2) to (3) even if we can figure out a way to do (3) because it makes it clear you're actually getting back the original object for chaining purposes. In cases where it is a copy / different instance, we should explicitly say so somehow probably, maybe with another name like
copied instance of same typeor something. But we'd need to think about how to do this in a NumpyDoc-acceptable manner.
I agree that (2) is the best option, though IMO it is pretty tricky to come up with simple, unambiguous wording. Here's an (unsatisfactory) attempt:
Returns
-------
inst : self
An instance of the object type whose method was called. If ``copy=True``
this will be a copy with <WHATEVER THE METHOD CHANGED>, otherwise the
original instance will be modified in-place and returned.
What about:
Returns
-------
inst : self
The modified object if ``copy=False``. If ``copy=True``,
this will be a copy with <WHATEVER THE METHOD CHANGED>.
I'd also drop the name of the return value if there is only one, i.e.
Returns
-------
self
The modified object if ``copy=False``. If ``copy=True``,
this will be a copy with <WHATEVER THE METHOD CHANGED>.
I'm not 100% happy with this either.
Returns ------- self
I don't think that's very helpful to non-developers.
Why can we not just spell it out?
Returns
-------
Raw, Epochs, or Evoked – same as input type
or so, for a function; and be explicit for a method and only list the one type that it applies to. We're not slaves of an auto-populated docstring!!
and be explicit for a method and only list the one type that it applies to
because for methods that come from mixins, we don't know the type at the time the doc is being written. Hence the mention of "the super class doc" in the OP.
and be explicit for a method and only list the one type that it applies to
because for methods that come from mixins, we don't know the type at the time the doc is being written. Hence the mention of "the super class doc" in the OP.
Then how about the poor person's approach of
Returns
-------
result : same type as the input data
?
result : same type as the input data
that's not a bad proposal in terms of clarity, but whatever we do has to pass numpydoc and/or be special-cased so that it passes numpydoc. Therefore fewer words are better, and I'm a bit hesitant to special-case words like input and data.
result : same type as the input data
that's not a bad proposal in terms of clarity, but whatever we do has to pass
numpydocand/or be special-cased so that it passesnumpydoc. Therefore fewer words are better, and I'm a bit hesitant to special-case words likeinputanddata.
How about
result : same type as inst
?
Edit: Argh no, this doesn't work with methods from a MixIn
@hoechenberger I see you milestoned this for 1.2 but we need to converge and have a volunteer to work on this. I think it's not likely to happen in the next ~week