`docstring.py` assumes that a method of class should NOT return a value

[NimaSarajpoor]

If a method (of a class) returns a value, then the else part in the following lines from docstring.py:

stumpy/docstring.py

Lines 26 to 33 in ce05903

	if class_name is None:
	params_section = re.findall(
	r"(?<=Parameters)(.*)(?=Returns)", docstring, re.DOTALL
	)[0]
	else:
	params_section = re.findall(r"(?<=Parameters)(.*)", docstring, re.DOTALL)[0]
	args = re.findall(r"(\w+)\s+\:", params_section)

captures that output variable name from its docstring and includes it in the args. This issue was initially exposed when a class with method __call__ was added in PR #1118. One suggestion provided in this comment was:

Given that this is an extreme case, it almost feels like it merits its on elif condition that specifically handles the specific _call_ case?

I think the real question we should ask ourselves is: "Should we treat methods like regular functions, meaning they can return value?

I understand that all methods of all classes in STUMPY main branch currently return nothing ("None"). However, I think it is not unreasonable to assume that, in future, a class might be added that has a method that returns a value. So, maybe we should revise the else part in the code above ??

[seanlaw]

I understand that all methods of all classes in STUMPY main branch currently return nothing ("None"). However, I think it is not unreasonable to assume that, in future, a class might be added that has a method that returns a value.

I think that the __call__ is still an extreme example simply because we are trying to make the class "callable" (which is nice and I like this!). Technically, we have many, many class methods in STUMPY that return a value. For example:

stumpy/stumpy/stimp.py

Lines 364 to 382 in 6f0169a

	@property
	def P_(self):
	"""
	Get all of the raw (i.e., non-transformed) matrix profiles matrix profile in
	(breadth first searched (level) ordered)
	Parameters
	----------
	None
	Returns
	-------
	None
	"""
	P = []
	for i, idx in enumerate(self._bfs_indices):
	P.append(self._PAN[idx][: len(self._T) - self._M[i] + 1])
	return P

However, notice that while there is a clear return value in the method, we've simply but:

        Returns
        -------
        None

in the Docstring. So, the class methods do and can return non-None BUT the Docstring isn't reflecting the correct reality!

So, maybe we should revise the else part in the code above ??

If I understand correctly, the current else handles docstrings in classes and, even though some class methods have a Returns section (some like the stimp class have a return value of None), the Return and None are still getting shoved into params_section (this is true even without the sdp class!).

Frankly, I cannot recall why I differentiated between a class and a non-class because the two regexes look very similar except the non-class condition has an additional (?=Returns) at the end that tells the regex where to stop looking while the class condition keeps searching the entire docstring. There must be a rational reason for this!

It looks like docstring.py recognizes a class' def __init__(self,...) method as a valid function and attempts to read it. __init__ methods rarely ever have return values so we stop searching for parameters by scanning all the way until the end of the docstring. Based on this observation, the key question is "Given a method/function docstring, when precisely should we stop searching for parameters?". In the case of a function, the Parameters section is always proceeded IMMEDIATELY by the Returns section so we can safely stop and only capture the parameters. This is NOT true for class methods! Thus, for docstrings in class methods, we don't know (logically) when to stop searching the docstring besides reading until the end of the docstring!

Perhaps, the simple solution is to ALWAYS add a Returns section to every class method and then we wouldn't need to differentiate between a function and a class method?

[NimaSarajpoor]

we have many, many class methods in STUMPY that return a value. For example:

I missed that...probably because I was just looking at the "Returns" section of docstrings

However, notice that while there is a clear return value in the method, we've simply but:

Returns
-------
None

the Return and None are still getting shoved into params_section (this is true even without the sdp class!).

Correct. And because it does not have that :, the next line, i.e. args = re.findall(r"(\w+)\s+\:", params_section) gives the correct result.

In the case of a function, the Parameters section is always proceeded IMMEDIATELY by the Returns section so we can safely stop and only capture the parameters. This is NOT true for class methods!

Perhaps, the simple solution is to ALWAYS add a Returns section to every class method and then we wouldn't need to differentiate between a function and a class method?

Maybe that was the initial intention behind adding "Returns" section to all methods of classes in STUMPY. I think the only case we need to handle is the method __init__ ... even in that case, as you pointed out, if we add the section "Returns" , we should be fine (and that will cover the rare case __call__ too)

The whole if-else block can then be replaced with:

params_section = re.findall( 
         r"(?<=Parameters)(.*)(?=Returns)", docstring, re.DOTALL 
     )[0] 

[seanlaw]

@NimaSarajpoor I will work on a quick PR for this!