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.

Sign up for GitHub

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

Jump to bottom

Scanbox format updates #1311

Merged
merged 7 commits into from
Mar 29, 2024
Merged

Scanbox format updates #1311

merged 7 commits into from
Mar 29, 2024

Conversation

ethanbb
Copy link
Contributor

@ethanbb ethanbb commented Mar 29, 2024 •
edited
Loading

Description

  • Updates .sbx loading code to deal with newer versions of the format;
  • Adds functions to convert .sbx files and chains thereof to .tifs for more flexibility;
  • Adds a function to load the metadata;
  • Pulls all the sbx-relevant code into a separate file under utils.

Fixes #1299

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Has your PR been tested?

Yes, tests for .sbx format added to tests/test_sbx.py, using example datafiles added to testdata
All tests passed with caimanmanager test. caimanmanager demotest fails because of #1310.

@ethanbb ethanbb changed the title Sbxload update Scanbox format updates Mar 29, 2024
pgunn
pgunn reviewed Mar 29, 2024
View reviewed changes
Copy link
Member

@pgunn pgunn left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. I particularly like your type annotations style; might try out some of that with my own code because of how clean it looks. Left a few little comments.

caiman/base/movies.py Outdated
Comment on lines 37 to 38
import caiman.utils.visualization
import caiman.utils.sbx_utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we flip the order to keep these alphabetised?

caiman/base/movies.py Outdated
return movie(sbxreadskip(file_name[:-4], subindices), fr=fr).astype(outtype)
else:
return movie(sbxread(file_name[:-4], k=0, n_frames=np.inf), fr=fr).astype(outtype)
meta_data = caiman.utils.sbx_utils.sbx_meta_data(file_name[:-4])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a lot of code throughout the codebase that does this; eventually we'll want to convert them all to use os.path.splitext() instead of indexing into the string. When I fix them all I'll grep for stuff like this, so it's fine to leave this in (although if you want to adjust it yourself you can)

caiman/tests/test_sbx.py Outdated
Comment on lines 3 to 6
import os

import numpy as np
import numpy.testing as npt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we alphabetise these parts of the imports? (the caiman imports going later is fine

caiman/utils/sbx_utils.py Outdated
Comment on lines 7 to 13
import os
import logging
from typing import Iterable

import numpy as np
import scipy
import tifffile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we alphabetise these imports?

@ethanbb
Copy link
Contributor Author

ethanbb commented Mar 29, 2024

@pgunn thank you! No problem, I changed sbx_utils as well to not directly slice the path name.

@pgunn
Copy link
Member

pgunn commented Mar 29, 2024

Looking good, will merge once CI finishes. Thanks!

@pgunn
Copy link
Member

pgunn commented Mar 29, 2024

Looks like the CI pipeline is currently busted because someone added an OpenGL dependency to opencv. I'll merge this and follow up with the OpenCV people separately. Thanks for the patch!

@pgunn pgunn merged commit 16cb274 into flatironinstitute:dev Mar 29, 2024
0 of 3 checks passed
@pgunn
Copy link
Member

pgunn commented Mar 29, 2024

@ethanbb Ah, I'm fixing CI now and it turns out CI found an issue with the code:

  File "/home/runner/work/CaImAn/CaImAn/caiman/base/movies.py", line 37, in <module>
    import caiman.utils.sbx_utils
  File "/home/runner/work/CaImAn/CaImAn/caiman/utils/sbx_utils.py", line 435
    return chunk[:, *np.ix_(*subindices[1:])]
                    ^
SyntaxError: invalid syntax
Error: Process completed with exit code 1.

@ethanbb
Copy link
Contributor Author

ethanbb commented Mar 29, 2024

Hmm, do you know if this is a Python version issue? I did test it with Python 3.11 and it worked for me.

@pgunn
Copy link
Member

pgunn commented Mar 29, 2024

Looks like this is 3.11-specific syntax; is there a chance you can, for now, rephrase it to be compatible back to 3.9?

@ethanbb
Copy link
Contributor Author

ethanbb commented Mar 29, 2024

Yup, no problem.

@ethanbb ethanbb mentioned this pull request Mar 29, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgunn pgunn pgunn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ethanbb @pgunn