nel-lab / mesmerize-core

High level pandas-based API for batch analysis of Calcium Imaging data using CaImAn
Other
60 stars 15 forks source link

Don't filter out NaNs for contours #309

Open ethanbb opened 3 months ago

ethanbb commented 3 months ago

Fixes #308. @kushalkolar I believe we decided to go ahead with removing the line that filters out NaNs, but not change how centers of mass are calculated here (except for using nanmean).

ethanbb commented 2 months ago

The ground truths files need to be updated on Zenodo to reflect this change (as well as a recent bug fix in caiman). I went through the tests with a debugger and used the following code to check that each list of contours matched the existing ground truths after NaNs were removed and the fix was undone:

new_contours = []
for contour, actual_contour in zip(..., ...):
    new_contour_nansremoved = contour[~np.any(np.isnan(contour), axis=1), :]
    if not np.all(np.isclose(actual_contour[0, :], actual_contour[-1, :])):
        # account for previous bug if corner of image is a vertex
        new_contour_nansremoved = new_contour_nansremoved[1:]
    np.testing.assert_allclose(actual_contour, new_contour_nansremoved, rtol=1e-2, atol=1e-10)
    new_contours.append(contour)
np.save(ground_truths_dir.joinpath(...), np.array(new_contours, dtype='O'))

I also had to re-save one of the center-of mass files to account for the COM being different after the bugfix.

With this plus flatironinstitute/CaImAn#1387, all tests are passing!

Link to updated ground_truths.zip: https://upenn.box.com/s/fccm6jnrvk9yma9ns5eoxau5ttrzflsq

kushalkolar commented 2 months ago

Thanks! I guess this along with a new ground-truths file can be done in a PR once that caiman PR is in the next release?

ethanbb commented 2 months ago

Oh oops sorry I didn't mean to close the PR. Let me bring it back.

ethanbb commented 2 months ago

This doesn't depend on that caiman PR, it's just related to the last test that's still failing.

kushalkolar commented 2 months ago

ah ok, do you want to upload the new ground truths to zenodo and then modify the PR to retrieve from that new dataset link? I just tried to access the current ground truth upload and I have no idea who has the original access to it so you can just do one if you're ok with?

ethanbb commented 2 months ago

OK I will try!

kushalkolar commented 2 months ago

I think all you have to do is replace this line:

https://github.com/nel-lab/mesmerize-core/blob/master/tests/test_core.py#L47

ethanbb commented 2 months ago

Should be good now, but I think there is some problem setting up the CI environments?

kushalkolar commented 2 months ago

looking at the pip install first, issue with tensorflow and keras mismatch, will tell it to run again and hope it picks up correct versions now

With conda, it seems like the issue is that it's using python3.12 by default for some reason, can fix later or you can update the workflow yaml. Currently on a plane.

ethanbb commented 2 months ago

OK I fixed the conda issues in #315. It seems like the pip run is still having the same issue, though.

kushalkolar commented 2 months ago

now there's that coo_matrix error with CNMFE: https://github.com/nel-lab/mesmerize-core/actions/runs/10687918032/job/29626568835?pr=309#step:7:1220

pip workflow's errors are due to keras and tensorflow tantrums, can ignore

ethanbb commented 2 months ago

Yup, https://github.com/flatironinstitute/CaImAn/pull/1387 fixes that but hasn't been included in a release yet.

kushalkolar commented 2 months ago

let's wait until the next release so CI is fixed in this PR?