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]: TimeSeries with empty dataset #403

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

[New Check]: TimeSeries with empty dataset #403

wants to merge 3 commits into from

Conversation

alessandratrapani
Copy link
Collaborator

@alessandratrapani alessandratrapani commented Sep 18, 2023 •
edited
Loading

Related to Issue #324
add check function for empty dataset

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

@alessandratrapani alessandratrapani marked this pull request as draft September 18, 2023 09:22
@alessandratrapani alessandratrapani changed the title add check function for empty dataset [New Check]: TimeSeries with empty dataset Sep 18, 2023
@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 self-assigned this Sep 18, 2023
@CodyCBakerPhD
Copy link
Contributor

CodyCBakerPhD commented Sep 18, 2023 •
edited
Loading

@alessandratrapani Very minor thing, but it seems as if GitHub is not smart enough to recognize the keyphrase 'Related to Issue #...' when automatically adding the development issue link

I always do 'fix #...', I think I've seen 'resolve #...' work for others

No worries, though - it's easy to add it afterward through the web interface

@CodyCBakerPhD CodyCBakerPhD linked an issue Sep 18, 2023 that may be closed by this pull request
[Add Check]: Datasets are not empty #324
Open
2 tasks
CodyCBakerPhD
CodyCBakerPhD reviewed Sep 18, 2023
View reviewed changes
src/nwbinspector/checks/nwb_containers.py
Comment on lines +34 to +40
def is_empty_data(data):
if isinstance(data, ndarray):
return data.size == 0
elif isinstance(data, (list, tuple)):
return all(is_empty_data(item) for item in data)
else:
return not bool(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind if this were moved to the common utils to be used by any other check (or anyone else) going forward 😃

@CodyCBakerPhD
Copy link
Contributor

I want to say the basic structure looks good for checking the data field; there are perhaps some other cases we must consider

The best way to tell will be to add several tests (cases where it should pass without raising a message, cases where it should catch 'emptiness', skips on certain edge cases such as ImageSeries)

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

At least 1 approving review is required 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]: Datasets are not empty
2 participants
@alessandratrapani @CodyCBakerPhD