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

Fix removeBL for 3D movies #1363

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Conversation

ethanbb
Copy link
Contributor

@ethanbb ethanbb commented Jun 13, 2024

Description

This PR is mainly focused on making sure local_correlations_movie works for 3D movies, by adding an is3D flag for passing to caiman.load and also slightly modifying movie.removeBL. I also fixed other occurrences in local_correlations.py where caiman.load is used without an is3D flag; however, I have not tested whether all these functions actually work for 3D movies.

This fix is necessary to get mesmerize-core's motion correction routine to work for 3D data. But I'd understand if you want to make it a little more broad.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Has your PR been tested?

caimanmanager test and caimanmanager demotest pass. I tested manually by running some of my data through local_correlations_movie (both from mesmerize-core and otherwise).

@pgunn
Copy link
Member

pgunn commented Jun 13, 2024

It looks generally workable; I wonder if this might be improved by modifying the params object to notice and activate this automatically when the passed in dims has more cardinality (data.dims); right now there's some design tension between with passing params around and having it control behaviour versus passing parameters to various functions (this is a longer-term concern though).

I worry about the code being fragile to future refactoring without our noticing it.
If you can add a test that would exercise this functionality, that'd be good.

@ethanbb
Copy link
Contributor Author

ethanbb commented Jun 14, 2024

You know what, I don't think the is3D flag is necessary after all. I just looked again at the load function and realized that is3D is only used if the file name passed in is actually a list. This is happening in mesmerize-core, but it's against the documented api of local_correlations_movie_offline, so that call should just be changed.

The fixes to those reshaping utilities are still necessary, though. I can add tests for them.

.gitignore Outdated Show resolved Hide resolved
@ethanbb ethanbb changed the title Fix certain errors in summary images with 3D movies Fix removeBL for 3D movies Jun 14, 2024
@ethanbb
Copy link
Contributor Author

ethanbb commented Jun 14, 2024

Also, slightly off-topic now but:

if channel is not None:
    logging.debug(m.shape)
    m = m[channel].squeeze()
    logging.debug(f"Movie shape: {m.shape}")

is this clause in load_movie_chain ever used at this point? Since as I understand the output of load should not have a channel dimension (first dimension of any movie should be time, right?) And if not, if we're never squeezing the movie, do we need is3D at all; can't we infer it from the dimensionality of the movie(s)?

@pgunn
Copy link
Member

pgunn commented Jun 14, 2024

I'll look at this; the code has a lot of things in it that are left over from either prior designs or being overly cautious/flexible.

I'm about to leave for my conference though - I might not have an answer before I get back.

@pgunn
Copy link
Member

pgunn commented Jul 1, 2024

Also, slightly off-topic now but:

if channel is not None:
    logging.debug(m.shape)
    m = m[channel].squeeze()
    logging.debug(f"Movie shape: {m.shape}")

is this clause in load_movie_chain ever used at this point? Since as I understand the output of load should not have a channel dimension (first dimension of any movie should be time, right?)

We seem to have no current code that uses channel; I'm guessing it was added to support some people who had their data in an unusual format, and we probably should tell people in that case to convert first. I'm going to look back through repo history to see when this was added (assuming it wasn't there from the start) to get a bit more history.

@pgunn
Copy link
Member

pgunn commented Jul 1, 2024

Also, slightly off-topic now but:

if channel is not None:
    logging.debug(m.shape)
    m = m[channel].squeeze()
    logging.debug(f"Movie shape: {m.shape}")

is this clause in load_movie_chain ever used at this point? Since as I understand the output of load should not have a channel dimension (first dimension of any movie should be time, right?)

We seem to have no current code that uses channel; I'm guessing it was added to support some people who had their data in an unusual format, and we probably should tell people in that case to convert first. I'm going to look back through repo history to see when this was added (assuming it wasn't there from the start) to get a bit more history.

The channel argument was added to load_movie_chain() on 18 September 2017, in a large diff called "pre merging 1photon with master"; ( 02d7977 ) this was probably a squashed merge so we don't have history for it.

The channel argument was added to load() on 8 December 2017 in a diff called "multiple file support" that made direct use of load_movie_chain() usually unnecessary because load() would call it for you if you passed a list (pity we never took that principle all the way; maybe something for the future).

It looks like pipeline_analysis from the paper used to use this argument, with an internal tiff dataset (we never shipped it and it was only present on the shared filesystem at the Flatiron Institute).

@ethanbb
Copy link
Contributor Author

ethanbb commented Jul 1, 2024

Hey, thanks for taking a look. I more recently made a discussion #1366 about this channel argument - instead of removing, maybe we should make it more functional? Or what is the alternative for multi-channel recordings?

@pgunn
Copy link
Member

pgunn commented Jul 1, 2024

Hey, thanks for taking a look. I more recently made a discussion #1366 about this channel argument - instead of removing, maybe we should make it more functional? Or what is the alternative for multi-channel recordings?

I think the alternative would probably be for people to pre-convert their data out of such formats.

If you'd like to convert it to behave differently, I'm open to that; otherwise I'm inclined to just remove it.

@ethanbb
Copy link
Contributor Author

ethanbb commented Jul 24, 2024

@pgunn I think the discussion got a little off track... this is a pretty minor change, do you think we could push this through soon to unblock the corresponding issue in mesmerize-core?
Thanks!

@pgunn pgunn merged commit 0dac913 into flatironinstitute:dev Jul 24, 2024
3 checks passed
@pgunn
Copy link
Member

pgunn commented Jul 24, 2024

Sure

@ethanbb ethanbb deleted the summary-images-3d branch July 24, 2024 19:51
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

Successfully merging this pull request may close these issues.

3 participants