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

Add npy support to load_memmap #1446

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Add npy support to load_memmap #1446

merged 2 commits into from
Jan 10, 2025

Conversation

fdeguire03
Copy link
Contributor

Description

The caiman.mmapping.load_memmap function currently supports only .mmap files. Since .npy files store data in the same format as .mmap files, they can be used interchangeably with minor adjustments during loading (due to the header present in .npy files).

This update extends load_memmap to support .npy files, enabling their use in the CNMF parallelization framework.

Fixes #1431

Type of change

Please delete options that are not relevant.

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

Branching

  • All PRs should be made against the dev branch. The main branch is not often merged back to dev.
  • If you want to get your PR out to the world faster (urgent bugfix), poke pgunn to cut a release; this will get it onto github and into conda faster

Has your PR been tested?

If you're fixing a bug or introducing a new feature it is recommended you run the tests by typing

caimanmanager test

and

caimanmanager demotest

prior to submitting your pull request.

Please describe any additional tests that you ran to verify your changes. If they are fast you can also
include them in the folder 'caiman/tests/and name themtest_***.py` so they can be included in our lists of tests.

No additional tests were added, as the change modifies internal behavior without introducing new user-facing functionality or regressions. Existing test coverage should adequately validate this change.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@pgunn
Copy link
Member

pgunn commented Jan 10, 2025

One small thing I'm worried about is if the user provides data that's not an np.float32; I wonder if it's worth adding checks for that and forbidding it (at the very least it'd be a good line in the sand to tell users we're not committed to supporting data in alternate data depths)

@pgunn
Copy link
Member

pgunn commented Jan 10, 2025

Do you mind adding a quick check on the data format and throwing an exception if it's not an np.float32?

@fdeguire03
Copy link
Contributor Author

Sure thing. I added checks for all of the items that are being passed in the np.memmap constructor but are now contained in the npy header -- dtype, array shape, and memory order. Ran some quick tests on my end and confirmed that the function throws an error if the npy file header disagrees with any of the attributes inferred from the filename. Let me know what you think.

@pgunn
Copy link
Member

pgunn commented Jan 10, 2025

Will merge when it goes green.

@pgunn pgunn merged commit beb0f9b into flatironinstitute:dev Jan 10, 2025
3 checks passed
@fdeguire03
Copy link
Contributor Author

Great! Should be good to close #1431 now?

@pgunn
Copy link
Member

pgunn commented Jan 10, 2025

Yup

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.

None yet

2 participants