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

Don't filter out NaNs for contours #309

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ethanbb
Copy link
Collaborator

@ethanbb ethanbb commented Jul 24, 2024

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
Copy link
Collaborator Author

ethanbb commented Aug 11, 2024

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 ethanbb closed this Aug 11, 2024
@ethanbb ethanbb deleted the contour-nan-fix branch August 11, 2024 06:23
@kushalkolar
Copy link
Collaborator

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
Copy link
Collaborator Author

ethanbb commented Aug 11, 2024

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

@ethanbb ethanbb restored the contour-nan-fix branch August 11, 2024 06:35
@ethanbb ethanbb reopened this Aug 11, 2024
@ethanbb
Copy link
Collaborator Author

ethanbb commented Aug 11, 2024

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

@kushalkolar
Copy link
Collaborator

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
Copy link
Collaborator Author

ethanbb commented Aug 12, 2024

OK I will try!

@kushalkolar
Copy link
Collaborator

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
Copy link
Collaborator Author

ethanbb commented Aug 12, 2024

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

@kushalkolar
Copy link
Collaborator

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
Copy link
Collaborator Author

ethanbb commented Sep 1, 2024

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

@kushalkolar
Copy link
Collaborator

kushalkolar commented Sep 5, 2024

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
Copy link
Collaborator Author

ethanbb commented Sep 5, 2024

Yup, flatironinstitute/CaImAn#1387 fixes that but hasn't been included in a release yet.

@kushalkolar
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants