mesmerize-core icon indicating copy to clipboard operation
mesmerize-core copied to clipboard

Don't filter out NaNs for contours

Open ethanbb opened this issue 1 year ago • 13 comments

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 avatar Jul 24 '24 20:07 ethanbb

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

ethanbb avatar Aug 11 '24 06:08 ethanbb

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?

kushalkolar avatar Aug 11 '24 06:08 kushalkolar

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

ethanbb avatar Aug 11 '24 06:08 ethanbb

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

ethanbb avatar Aug 11 '24 06:08 ethanbb

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?

kushalkolar avatar Aug 12 '24 03:08 kushalkolar

OK I will try!

ethanbb avatar Aug 12 '24 03:08 ethanbb

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

kushalkolar avatar Aug 12 '24 03:08 kushalkolar

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

ethanbb avatar Aug 12 '24 20:08 ethanbb

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.

kushalkolar avatar Aug 31 '24 11:08 kushalkolar

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

ethanbb avatar Sep 01 '24 06:09 ethanbb

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

kushalkolar avatar Sep 05 '24 01:09 kushalkolar

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

ethanbb avatar Sep 05 '24 05:09 ethanbb

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

kushalkolar avatar Sep 06 '24 01:09 kushalkolar

wow tests are passing for the first time in year :open_mouth:

kushalkolar avatar Feb 27 '25 05:02 kushalkolar

Guessing the small error for comparing RCMs on macOS comes down to x86 vs ARM...I am willing to ignore Edit: or increase the tolerance only if the architecture is not x86, maybe

ethanbb avatar Feb 28 '25 06:02 ethanbb

Guessing the small error for comparing RCMs on macOS comes down to x86 vs ARM...I am willing to ignore Edit: or increase the tolerance only if the architecture is not x86, maybe

yea that's a common issue

kushalkolar avatar Feb 28 '25 06:02 kushalkolar