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

stub options #670

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

stub options #670

wants to merge 5 commits into from

Conversation

luiztauffer
Copy link
Member

small modification which allows for changing the sample size of the sub recording conversion

@CodyCBakerPhD
Copy link
Member

Can you describe what you need this for?

def subset_recording(self, stub_test: bool = False):
def subset_recording(self, stub_test_size: int = None):
Copy link
Member

Choose a reason for hiding this comment

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

Breaks back-compatability; please add deprecation cycle to stub_test

Comment on lines 305 to +306
stub_test: bool = False,
stub_test_size: int = 100,
Copy link
Member

Choose a reason for hiding this comment

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

I would also recommend calling this stub_frames or anything else to indicate it is not stubbing channels along the way (I know it's in the docstring, but if it's in the variable name that's even clearer)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use this already, don't we?

Comment on lines -365 to +371
recording = self.subset_recording(stub_test=stub_test)
recording = self.subset_recording(stub_test_size=stub_test_size)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment above; this would break all current scripts relying on stub_test as the syntax for testing conversions. Not to mention the GUIDE

@luiztauffer
Copy link
Member Author

hey @CodyCBakerPhD these are some changes I made locally to be able to create a converted file with a meaningful subset of the spikeglx recordings, I thought it could be useful in general
if you think this is a valid approach, we can work on your suggestions

@CodyCBakerPhD
Copy link
Member

hey @CodyCBakerPhD these are some changes I made locally to be able to create a converted file with a meaningful subset of the spikeglx recordings, I thought it could be useful in general

Were you attempting to make the preview files longer or shorter than the previous default stubbing?

Not against adding control to this in general, would just prefer it be done in a smooth way

In general the hope was actually for stubbing to be a part of the general backend configuration mechanism but that particular aspect proves a little tricky ATM so a bit delayed on that front

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4bef01d) 92.20% compared to head (8ab40d8) 92.18%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #670      +/-   ##
==========================================
- Coverage   92.20%   92.18%   -0.02%     
==========================================
  Files         115      115              
  Lines        5964     5966       +2     
==========================================
+ Hits         5499     5500       +1     
- Misses        465      466       +1     
Flag Coverage Δ
unittests 92.18% <80.00%> (-0.02%) ⬇️

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

Files Coverage Δ
...erfaces/ecephys/baserecordingextractorinterface.py 96.07% <80.00%> (-0.93%) ⬇️

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.

3 participants