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

Avoid having to set rate=None explitctly when passing timestamps in mock_ElectricalSeries #1894

Merged

Conversation

h-mayorquin
Copy link
Contributor

@h-mayorquin h-mayorquin commented Apr 25, 2024

Motivation

Right now you get the following error when trying to pass timestamps to the mock_ElectricalSeries function.

from pynwb.testing.mock.ecephys import mock_ElectricalSeries


mock_ElectricalSeries(timestamps=np.arange(10))
ValueError: Specifying rate and timestamps is not supported.

This is because we have rate=30,000.0 as a default value in the function signature. To make this work, the user has to explictly set rate=None, that is:

from pynwb.testing.mock.ecephys import mock_ElectricalSeries


mock_ElectricalSeries(timestamps=np.arange(10), rate=None)

This is too cumbersome. This PR simplifies this so this is not necessary. To do this, it moves the logic of default values inside the function.

I also added some missing typing.

How to test the behavior?

The code above should work.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@h-mayorquin h-mayorquin changed the title Set rate as optional value mock_ElectricalSeries Avoid having to set rate=None explitctly when passing timestamps in mock_ElectricalSeries Apr 25, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review April 25, 2024 15:57
@stephprince stephprince self-requested a review May 2, 2024 16:21
@stephprince stephprince self-assigned this May 2, 2024
@stephprince stephprince added this to the 2.8.0 milestone May 2, 2024
Copy link
Contributor

@stephprince stephprince left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review! Added a few small suggestions but otherwise looks good to me.

CHANGELOG.md Outdated Show resolved Hide resolved
src/pynwb/testing/mock/ecephys.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.93%. Comparing base (86b274d) to head (2df0f03).

❗ Current head 2df0f03 differs from pull request most recent head a5bfe19. Consider uploading reports for the commit a5bfe19 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1894      +/-   ##
==========================================
- Coverage   92.02%   83.93%   -8.10%     
==========================================
  Files          27       27              
  Lines        2620     2620              
  Branches      685      685              
==========================================
- Hits         2411     2199     -212     
- Misses        139      328     +189     
- Partials       70       93      +23     
Flag Coverage Δ
integration ?
unit 83.93% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stephprince
Copy link
Contributor

The failing checks are due to a separate hdmf-zarr issue hdmf-dev/hdmf-zarr#192 and are unrelated to these changes to mock_ElectricalSeries. So we will ignore them for now and go ahead and merge this PR.

@stephprince stephprince merged commit 7a0d8b4 into NeurodataWithoutBorders:dev May 14, 2024
14 of 21 checks passed
@h-mayorquin h-mayorquin deleted the make_rate_optional_in_mock branch May 14, 2024 11:41
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.

2 participants