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.

Sign up for GitHub

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

Jump to bottom

add inscopix imaging extractor #276

Merged
merged 33 commits into from
May 13, 2024
Merged

add inscopix imaging extractor #276

merged 33 commits into from
May 13, 2024

Conversation

bendichter
Copy link
Contributor

No description provided.

bendichter and others added 13 commits January 23, 2024 14:35
@bendichter
Copy link
Contributor Author

blocked by inscopix/pyisx#48

@pauladkisson
Copy link
Member

Looks like this is unblocked as of v1.0.4 (inscopix/pyisx#48 (comment))

@CodyCBakerPhD
Copy link
Member

Looks like the test cases just need to be updated now - also a CHANGELOG entry

CodyCBakerPhD
CodyCBakerPhD reviewed May 2, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@bendichter bendichter marked this pull request as ready for review May 6, 2024 16:45
@bendichter
Copy link
Contributor Author

This is finally working. Though I think dropping 3.8 should probably be a different PR

@CodyCBakerPhD CodyCBakerPhD requested a review from pauladkisson May 6, 2024 16:50
@bendichter bendichter marked this pull request as draft May 6, 2024 16:53
@CodyCBakerPhD
Copy link
Member

I'll defer to @pauladkisson for PR review but mostly looks good to me

Though I think dropping 3.8 should probably be a different PR

For the global drop of 3.8 I would agree, and then we can do it synchronously throughout the ecosystem across the next few weeks (I think we have a couple of months still)

However, you would still need various skips of isx stuff for 3.8 (such as in the CI installation step too) for this PR so maybe too much bother here?

Could do the 3.8 drop in separate PR, give that PR precedence, then come back to this?

@pauladkisson
Copy link
Member

Happy to review, but will have to wait until 5/20

@h-mayorquin h-mayorquin mentioned this pull request May 12, 2024
Merged
@h-mayorquin
Copy link
Collaborator

catalystneuro/neuroconv#853

@bendichter
Copy link
Contributor Author

depends on #325

@bendichter
Merge branch 'main' into inscopix_imaging
7392050 
# Conflicts:
#	.github/ISSUE_TEMPLATE/bug_report.yml
#	.github/workflows/run-tests.yml
#	CHANGELOG.md
@bendichter bendichter marked this pull request as ready for review May 13, 2024 01:33
@bendichter bendichter requested a review from h-mayorquin May 13, 2024 01:33
h-mayorquin
h-mayorquin reviewed May 13, 2024
View reviewed changes
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, my only request is that we avoid the read operation every time dtype is requested.

tests/test_inscopiximagingextractor.py Outdated Show resolved Hide resolved
src/roiextractors/extractors/inscopixextractors/inscopiximagingextractor.py Outdated Show resolved Hide resolved
tests/test_inscopiximagingextractor.py Show resolved Hide resolved
bendichter and others added 3 commits May 12, 2024 22:09
@bendichter bendichter requested a review from h-mayorquin May 13, 2024 02:24
@bendichter
Copy link
Contributor Author

@h-mayorquin I added a method as you suggested. I also added a test to ensure that the dtype of the data indeed matches this dtype extacted from the metadata (this did not match in a previous version of the isx package)

@codecov Codecov
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.57%. Comparing base (7aa2a63) to head (6f2f8d4).
Report is 21 commits behind head on main.

❗ Current head 6f2f8d4 differs from pull request most recent head a41d30c. Consider uploading reports for the commit a41d30c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #276      +/-   ##
==========================================
+ Coverage   79.37%   79.57%   +0.19%     
==========================================
  Files          39       41       +2     
  Lines        3069     3099      +30     
==========================================
+ Hits         2436     2466      +30     
  Misses        633      633
Flag Coverage Δ
unittests 79.57% <100.00%> (+0.19%) ⬆️

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

Files Coverage Δ
src/roiextractors/extractorlist.py 100.00% <100.00%> (ø)
...tractors/extractors/inscopixextractors/__init__.py 100.00% <100.00%> (ø)
...ors/inscopixextractors/inscopiximagingextractor.py 100.00% <100.00%> (ø)

h-mayorquin
h-mayorquin approved these changes May 13, 2024
View reviewed changes
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right.

@bendichter bendichter merged commit 22ccdad into main May 13, 2024
25 checks passed
@bendichter bendichter deleted the inscopix_imaging branch May 13, 2024 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodyCBakerPhD CodyCBakerPhD CodyCBakerPhD left review comments

@h-mayorquin h-mayorquin h-mayorquin approved these changes

@pauladkisson pauladkisson Awaiting requested review from pauladkisson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bendichter @pauladkisson @CodyCBakerPhD @h-mayorquin