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

[New Check]: Spike times are increasing #405

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

[New Check]: Spike times are increasing #405

wants to merge 11 commits into from

Conversation

alessandratrapani
Copy link
Collaborator

@alessandratrapani alessandratrapani commented Sep 18, 2023 •
edited by CodyCBakerPhD
Loading

Relate to Issue #394
Add check function for ascending spike times

  • add check function
  • add unit tests
  • add check function documentation
  • update CHANGELOG.md

@alessandratrapani alessandratrapani added the category: new check a new best practices check to apply to all NWBFiles and their contents label Sep 18, 2023
@alessandratrapani alessandratrapani changed the title Fix issue #394 check if spike times are increasing [New Check]: Spike times are increasing Sep 18, 2023
@alessandratrapani alessandratrapani self-assigned this Sep 18, 2023
@CodyCBakerPhD CodyCBakerPhD linked an issue Sep 18, 2023 that may be closed by this pull request
[Add Check]: Check if spike times are increasing #394
Open
2 tasks
@bendichter bendichter removed a link to an issue Sep 18, 2023
[Add Check]: Check if spike times are increasing #394
Open
2 tasks
@bendichter bendichter linked an issue Sep 18, 2023 that may be closed by this pull request
[Add Check]: Check if spike times are increasing #394
Open
2 tasks
@bendichter
Copy link
Contributor

Thanks @alessandratrapani. We also need units tests

@bendichter bendichter marked this pull request as ready for review September 25, 2023 16:56
@bendichter
Copy link
Contributor

@alessandratrapani can you please add an entry here explaining this best practice in plain English?

@bendichter bendichter self-requested a review September 29, 2023 14:08
bendichter
bendichter approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@bendichter bendichter left a comment

Choose a reason for hiding this comment

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

lgtm!

@bendichter
Copy link
Contributor

@alessandratrapani , can you resolve the conflicts?

image

CodyCBakerPhD
CodyCBakerPhD requested changes Sep 29, 2023
View reviewed changes
src/nwbinspector/checks/ecephys.py


@register_check(importance=Importance.BEST_PRACTICE_VIOLATION, neurodata_type=Units)
def check_ascending_spike_times(units_table: Units):
Copy link
Contributor

Choose a reason for hiding this comment

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

We are still testing the performance of this, since it's the first data heavy check to be added without options to control how much data is loaded

Copy link
Contributor

Choose a reason for hiding this comment

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

@alessandratrapani Can you add a nelems option here to mirror other table tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @alessandratrapani

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodyCBakerPhD CodyCBakerPhD CodyCBakerPhD requested changes

@bendichter bendichter bendichter approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@alessandratrapani alessandratrapani

Labels
category: new check a new best practices check to apply to all NWBFiles and their contents
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Add Check]: Check if spike times are increasing
3 participants
@alessandratrapani @bendichter @CodyCBakerPhD