Upcoming return value changes

[pgunn]

Hello,
I am working on some changes across the codebase that will remove the return values from methods that:
A) Modify their object inline, and
B) Return self

In such cases, the return values have little use, only permitting the mild convenience of allowing the chaining of multiple methods together on one line of code, and having the disadvantage of it being unclear whether what they return is a distinct object from the the object used for the call (we also have methods that return a distinct object with data cloned from the current object; this is confusing).

One example of this is the CNMF object, which has methods like fit() and compute_residuals() that return self after doing their work.

I had to modify the demos and some test code, where the calling code often looked like this:

cnm = cnm.fit(images)

After this change, it will need to look like

cnm.fit(images)

with no assignment (which was never necessary anyway).

This is a breaking change; if you have modified notebooks, when you next upgrade Caiman (after what will probably be the next release) you will need to change them (but the changes are thankfully trivial). You could also change them now without changing the function of your code.

[rob-the-bot]

Thanks for letting us know. However returning self after fit is standard practice in many Python packages such as sklearn. This is also done in other packages from Flatiron Institute such as NeMoS.

Removing the return value in fit is such a breaking change, I wonder if it makes more sense to go the other way—modifying the methods which involves data cloning. What are the thoughts from other people?

[pgunn]

We've speculated that it may have originally been done this way to follow sklearn conventions, although not many other sklearn conventions were followed.

I don't think we really need to do this change, it's just getting rod of an unusual code smell.

Right now I don't think the return-self convention benefits the code much - the ability to chain methods isn't something we use in our demos nor is it something that sounds like a good idea (making a one-liner with many method calls seems bad for easy reading of code). For the other methods that return self, nothing in our code looks at the return code, and the refactor is pretty easy.

Is there something particularly useful you find in fit() returning self? It's possible I'm overlooking something.

[rob-the-bot]

Personally I don't use the return value nor do I chain methods with caiman. I'm just weighing the cons here: over the many years of caiman, the return values from cnm.fit has also been "used". Implementing such change will make fit return None, which will very likely break the current and previous versions of the notebook, if there are have something as simple as cnm = cnm.fit(images). Since there is an unknown number of users who are using the current versions of the notebooks or modified versions from it (or the previous versions of notebooks), this can be a big breaking change.

I think it makes sense to remove the return self from some of the more low-level functions which are less-used by users, for example compute_residuals or deconvolve.

I'd like to keep the return self from higher-level functions such as fit, and other functions whose return values were heavily used throughout the history of the example notebooks.

[pgunn]

We've made breaking changes before, many of which we're far better off for having done; we shouldn't be afraid to make breaking changes if we think it makes the code better, and at least with this change the fix is pretty simple.

FYI, this is the WIP diff so far:
https://github.com/flatironinstitute/CaImAn/pull/1464/files

If there's a functionality reason to keep the code as-is I'm open to it, but I'm not keen on a commitment to never change APIs.

[pgunn]

(I'll also talk with the nemos author and see if he has thoughts)

[rob-the-bot]

Thanks!

[pgunn]

I talked with him; we dug into what you can do if your code offers methods that take references to other methods, with things that look like closures, but Caiman doesn't support that model right now anyhow; none of our methods take code refs and return more code refs.

So far I think the only thing we'd lose, with the current codebase and this revision, is the ability to do something like cnmf.fit(SOMETHING).something_else().something_yet_else()

If there's something else I'm missing I'd be happy to consider it.

[kushalkolar]

sklearn's Gridsearch and other "meta" model features are based on sklearn Estimators returning self. But caiman doesn't follow any of the other requirements that sklearn requires to use their meta features so they can't be used with caiman anyways.