Allow np.bool_ as a valid type when bool is specified in spec

[dsleiter]

• Allow np.bool_ as a valid type when bool is specified in a spec
• Fix Validator should allow np.bool_ when validating against a bool spec #504
• Ref problem validating bool NeurodataWithoutBorders/pynwb#1298
• Include bool as a scalar type for docval

Motivation

To fix this pynwb issue: NeurodataWithoutBorders/pynwb#1298

As it is currently possible to create an attribute with a np.bool_ when the spec lists the dtype as bool, the validator should similarly allow np.bool_ as a valid bool type.

While testing, I discovered that a DatasetBulder will not accept an np.bool_ as valid data when the dtype is bool because bool is not included as a scalar type. This seemed like an inconsistency to me, so I added it as a scalar type for docval, but please let me know if you disagree and I'll revert the change.

How to test the behavior?

import numpy as np
from hdmf.spec import GroupSpec, DatasetSpec, AttributeSpec, SpecCatalog, SpecNamespace
from hdmf.build import GroupBuilder, DatasetBuilder
from hdmf.validate import ValidatorMap


def set_up_vmap(dtype):
    spec_catalog = SpecCatalog()
    spec = GroupSpec('A test group specification with a data type',
                     data_type_def='Bar',
                     datasets=[DatasetSpec('an example dataset', dtype, name='data')],
                     attributes=[AttributeSpec('attr1', 'an example attribute', dtype)])
    spec_catalog.register_spec(spec, 'test.yaml')
    namespace = SpecNamespace(
        'a test namespace', 'test_namespace', [{'source': 'test.yaml'}], version='0.1.0', catalog=spec_catalog)
    return ValidatorMap(namespace)


vmap = set_up_vmap('bool')
value = np.bool_(True)
bar_builder = GroupBuilder('my_bar',
                           attributes={'data_type': 'Bar', 'attr1': value},
                           datasets=[DatasetBuilder('data', value)])
vmap.validate(bar_builder)

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.

[dsleiter]

Per the comment here, I tried to maintain consistency between the type maps listed. However, it is very possible that there is some side-effect from adding support for np.bool_ that I am not aware of, so I would appreciate if someone could do a double-check for me.

[rly]

Suggested change

	'bool': ["bool", "bool_"],
	'bool': ["bool"],

This change is not necessary unless we want to add a new alias "bool_" for "bool" for dtype specs in the spec yaml.

[dsleiter]

Ah, ok, that makes the purpose of primary_dtype_synonyms more clear, thanks for clarifying.

I added bool_ here in order to have it added to validator.__allowable (see https://github.com/agencyenterprise/hdmf/blob/enh/np_bool_in_validator/src/hdmf/validate/validator.py#L18-L40), since that seemed like the most minimally invasive way to resolve the issue, but I didn't know the side-effects of updating primary_dtype_synonyms. Removing bool_ here will make the new test fail, so we need to find another solution.

I originally wanted to do something like this: #203 but that would either require a more significant change to how the validator checks types in check_type (currently by matching against lists in __allowable), or make bool a unique case.

I've found another implementation here which I think is a good middle-ground and relatively consistent with the existing implementation. Let me know what you think.

For the longer term - do you think this solution is acceptable, or should we add an issue to try and bring the type checking in the validator in line with the method used for type checking in docval (#203)?

[rly]

Your change looks good and is where I would put it as well. Thanks!

[dsleiter]

Awesome, thanks @rly!