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

Ensure all specs are validated #507

Closed
4 of 5 tasks
dsleiter opened this issue Jan 12, 2021 · 7 comments · Fixed by #526
Closed
4 of 5 tasks

Ensure all specs are validated #507

dsleiter opened this issue Jan 12, 2021 · 7 comments · Fixed by #526
Labels
category: bug errors in the code or code behavior

Comments

@dsleiter
Copy link
Contributor

Description

Currently, there are a few edge cases where specs do not get validated, as raised in #500. We want to ensure that all specs get correctly validated.

  1. if a group includes two or more children with the same defined data type but a different name (independent of the quantity), then only the last instance will be validated. Eg. for an electrodes group that has VectorData X, Y, and Z - only Z will be validated. The validator should validate each of these children.
  2. if a child link has the same data type as a child group or dataset, then the link will be validated but the group or dataset with the same data type will not be validated. The validator should validate both the link and the group or dataset.
  3. if a group specifies two unnamed child groups of type A and B, where B inherits from A, then a single child of type B satisfies the spec for both B and A. The validator should require distinct children to match each spec. Both of the following cases should satisfy the spec: a) one child of type A and one of type B, or b) two children of type B.

Steps to Reproduce

Create an hdmf object and spec which should be invalid according to one of the three points above, and see that it passes validation.

Environment

Python Executable: Conda or Python
Python Version: Python 3.5, 3.6, 3.7, or 3.8
Operating System: Windows, macOS or Linux
HDMF Version: 2.3.0

Checklist

  • Have you ensured the bug was not already reported ?
  • Have you included a brief and descriptive title?
  • Have you included a clear description of the problem you are trying to solve?
  • Have you included a minimal code snippet that reproduces the issue you are encountering?
  • Have you checked our Contributing document?
@dsleiter dsleiter added the category: bug errors in the code or code behavior label Jan 12, 2021
@dsleiter
Copy link
Contributor Author

A solution for this issue should also validate correct regarding the discussion here: https://github.com/hdmf-dev/hdmf/pull/500/files#r555552469

@rly
Copy link
Contributor

rly commented Jan 13, 2021

Thanks for writing this up, @dsleiter. Would you be able to work on this?

@dsleiter
Copy link
Contributor Author

@rly sure! I'll get started on it this week.

@dsleiter
Copy link
Contributor Author

@rly I ran into another question while working on this. It looks like the validator does not currently check for additional attributes, datasets, groups, or links attached to a builder that are not part of the spec.

eg. imagine a group spec for type Foo defines two child datasets: X, and Y. Validation of a group builder for Foo with three child datasets X, Y, and Z will not return any validation errors.

Is this the desired intended behavior? I wasn't able to find anything specific to this in the hdmf documentation, but I might have missed it. I did find discussion related to this in nwb-schema docs which suggests that extending data types is the intended way of adding additional elements (last paragraph of section 1.3 here: https://nwb-schema.readthedocs.io/en/latest/format_description.html#time-series-a-base-neurodata-type-for-storing-time-series-data)

@rly
Copy link
Contributor

rly commented Feb 12, 2021

@dsleiter sorry for the delay in response.

This is related to:
https://github.com/NeurodataWithoutBorders/nwb-schema/pull/468/files
NeurodataWithoutBorders/pynwb#1332
NeurodataWithoutBorders/pynwb#942
NeurodataWithoutBorders/pynwb#1178
#288

In short, a warning should be raised on extra attributes/datasets/groups/links that are not part of the spec.

If you want to tackle that, you could finish #288 or start your own PR to do it. As you can see from the various issues raised about it, this issue does come up and it would be very useful to add a warning on extra fields into the validator.

@t-b
Copy link
Contributor

t-b commented Feb 12, 2021

Yes this would be definitly helpful in getting finished.

@dsleiter
Copy link
Contributor Author

Oh, great thanks! I should have checked the existing issues first.

I'd be happy to continue on #288 after this current issue. I think that the refactoring of GroupValidator I'm working on here will facilitate identifying superfluous objects, so I think it makes sense to complete this one first.

dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 24, 2021
* Ref hdmf-dev#507
* This property facilitates validating builders against specs
dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 24, 2021
dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 24, 2021
* Ref hdmf-dev#507
* Resolves the first two points of hdmf-dev#507
* Many comments were added during refactoring to help with understanding
  these comments will be removed before the branch is ready to merge
dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 25, 2021
* Ref hdmf-dev#507
* These unit tests are not currently passing but should by by the end of this issue
dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 25, 2021
* Ref hdmf-dev#507
* Now passes all tests
* The code will be further refactored and clean up before it is ready
dsleiter added a commit to agencyenterprise/hdmf that referenced this issue Feb 25, 2021
@rly rly closed this as completed in #526 Feb 26, 2021
rly pushed a commit that referenced this issue Feb 26, 2021
* Include data_type property on dataset and group specs

* Ref #507
* This property facilitates validating builders against specs

* Include additional validator unit tests

* Ref #507

* WIP Refactor GroupValidator and update logic to pass unit tests

* Ref #507
* Resolves the first two points of #507
* Many comments were added during refactoring to help with understanding
  these comments will be removed before the branch is ready to merge

* Add new unit tests for the validator

* Ref #507
* These unit tests are not currently passing but should by by the end of this issue

* Refactor GroupValidator and update matching between builder and spec

* Ref #507
* Now passes all tests
* The code will be further refactored and clean up before it is ready

* Refactor GroupValidator and SpecMatcher and add documentation

* Ref #507

* Clean up extraneous comments

* Refactor out IllegalLinkError creation to be consistent with other error creation

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants