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

Background Components for Caiman #783

Merged
merged 33 commits into from
May 3, 2024
Merged

Background Components for Caiman #783

merged 33 commits into from
May 3, 2024

Conversation

pauladkisson
Copy link
Member

@pauladkisson pauladkisson commented Mar 18, 2024

Fixes #782

Follow-up from ROIExtractors #291

@pauladkisson pauladkisson marked this pull request as ready for review March 29, 2024 22:17
@CodyCBakerPhD
Copy link
Member

pauladkisson requested review from CodyCBakerPhD and alessandratrapani 13 minutes ago

@pauladkisson Tests are failing at installation - can you update the pin?

@pauladkisson pauladkisson marked this pull request as ready for review April 4, 2024 01:29
@pauladkisson
Copy link
Member Author

@CodyCBakerPhD It's ready now

@CodyCBakerPhD
Copy link
Member

LGTM but Alessio was requesting a quick rerelease following some bug fixes to the backend configuration and we wouldn't be able to do that until the ROIExtractors pin is released

So I guess question is, how soon can we get an ROIExtractors release?

@pauladkisson
Copy link
Member Author

I think we can cut a release of ROIExtractors right away, but I'd like @EricThomson to take a look before we merge as well.

@pauladkisson pauladkisson marked this pull request as draft April 11, 2024 16:15
@pauladkisson pauladkisson marked this pull request as ready for review May 3, 2024 18:05
@pauladkisson pauladkisson requested a review from CodyCBakerPhD May 3, 2024 18:05
@pauladkisson
Copy link
Member Author

No response from Eric Thompson, but I don't want to leave this hanging forever, so I think we should go ahead with this and he can raise an issue if he runs into any problems with it.

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) May 3, 2024 18:16
@CodyCBakerPhD
Copy link
Member

@pauladkisson Does look like one test case broke a while ago

@CodyCBakerPhD CodyCBakerPhD self-requested a review May 3, 2024 18:38
@pauladkisson
Copy link
Member Author

pauladkisson commented May 3, 2024

Yeah, it looks like the conversion_options_schema doesn't like None for mask_type...

I tracked it down to src/neuroconv/utils/json_schema.py,

annotation_json_type_map = dict(
        bool="boolean",
        str="string",
        int="number",
        float="number",
        dict="object",
        list="array",
        tuple="array",
        FilePathType="string",
        FolderPathType="string",
    )

Apparently None isn't a valid type? I don't understand why we would annotate add_to_nwbfile as if you can provide none, if the schema doesn't actually let you do it. @CodyCBakerPhD, can you shed some light on this?

It does seem a bit outside of the scope of this PR tho.

@CodyCBakerPhD
Copy link
Member

This has been known since #530 and would be resolved with the Pydantic integration for handling all schema validations and signature parsing - for now I'd just comment out that one test case since we're already aware of it

@EricThomson
Copy link

Sorry I was away last month when that @ came in. I'm back and can look things over now (early next week) if it would help?

@pauladkisson
Copy link
Member Author

pauladkisson commented May 3, 2024

Sorry I was away last month when that @ came in. I'm back and can look things over now (early next week) if it would help?

Sure! That would be great! Just raise an issue and @ me if you run into any problems.

Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 93.10345% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.72%. Comparing base (a7b684a) to head (bbae94f).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
- Coverage   91.72%   91.72%   -0.01%     
==========================================
  Files         122      122              
  Lines        6682     6728      +46     
==========================================
+ Hits         6129     6171      +42     
- Misses        553      557       +4     
Flag Coverage Δ
unittests 91.72% <93.10%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rfaces/ophys/basesegmentationextractorinterface.py 100.00% <ø> (ø)
src/neuroconv/tools/roiextractors/__init__.py 100.00% <ø> (ø)
src/neuroconv/tools/roiextractors/roiextractors.py 88.02% <93.10%> (+0.39%) ⬆️

@CodyCBakerPhD CodyCBakerPhD merged commit d3cb683 into main May 3, 2024
43 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the caiman branch May 3, 2024 23:50
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.

[Feature]: Update CaimanSegmentationInterface to incorporate background components
3 participants