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

feat: Allow schema validation with tensor types #1665

Merged
merged 24 commits into from
Sep 9, 2022

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Oct 22, 2021

Pull Request Description

Teach pyhf.schema.validate a new trick!

Resolves #1662. This allows for schema validation to work with objects (specifications) that contain tensors in them. This is done by extending the base type checker to teach it about the tensor types from the backends used in pyhf. Should also help with validation for differentiability (#882).

Notes:

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Teach pyhf.schema.validate about the tensors from the backends.
* Define backend.array_type and backend.array_subtype to be used by the
  type checking in schema validation.
* Define pyhf.tensor.array_types and pyhf.tensor.array_subtypes to list
  all types currently loaded in the session.
* Add docstring example for pyhf.schema.load_schema.
* Add array information to docs class summary template.

@kratsg kratsg added feat/enhancement New feature or request fix A bug fix schema and spec JSON schema labels Oct 22, 2021
@kratsg kratsg self-assigned this Oct 22, 2021
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Base: 98.26% // Head: 98.28% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (7afc367) compared to base (d48f7f2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1665      +/-   ##
==========================================
+ Coverage   98.26%   98.28%   +0.01%     
==========================================
  Files          68       68              
  Lines        4440     4479      +39     
  Branches      728      730       +2     
==========================================
+ Hits         4363     4402      +39     
  Misses         45       45              
  Partials       32       32              
Flag Coverage Δ
contrib 27.61% <71.73%> (+0.38%) ⬆️
doctest 61.26% <91.30%> (+0.29%) ⬆️
unittests-3.10 96.22% <100.00%> (+0.03%) ⬆️
unittests-3.7 96.20% <100.00%> (+0.03%) ⬆️
unittests-3.8 96.24% <100.00%> (+0.03%) ⬆️
unittests-3.9 96.27% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/schema/loader.py 100.00% <ø> (ø)
src/pyhf/schema/validator.py 100.00% <100.00%> (ø)
src/pyhf/tensor/__init__.py 100.00% <100.00%> (ø)
src/pyhf/tensor/jax_backend.py 98.60% <100.00%> (+0.01%) ⬆️
src/pyhf/tensor/numpy_backend.py 98.61% <100.00%> (+0.01%) ⬆️
src/pyhf/tensor/pytorch_backend.py 98.48% <100.00%> (+0.02%) ⬆️
src/pyhf/tensor/tensorflow_backend.py 97.33% <100.00%> (+0.03%) ⬆️
src/pyhf/workspace.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kratsg kratsg force-pushed the feat/jsonschema_tensorValidation branch from 1d3c414 to 98bb2a2 Compare October 22, 2021 19:26
@matthewfeickert
Copy link
Member

This will probably be relevant sphinx-doc/sphinx#9479 given that the Sphinx v4.2.0 release notes include the note

#9479: autodoc: Emit a warning if target is a mocked object

@kratsg
Copy link
Contributor Author

kratsg commented Oct 25, 2021

This will probably be relevant sphinx-doc/sphinx#9479 given that the Sphinx v4.2.0 release notes include the note

Yes, but the target here is not a mocked object... So why is that relevant?

@matthewfeickert
Copy link
Member

Yes, but the target here is not a mocked object... So why is that relevant?

Because Sphinx thinks it is:

WARNING: A mocked object is detected: 'jax_backend.array_subtype'
WARNING: A mocked object is detected: 'jax_backend.array_type'
/opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/site-packages/pyhf/tensor/numpy_backend.py:docstring of pyhf.tensor.numpy_backend.numpy_backend.shape:1: WARNING: duplicate object description of pyhf.tensor.numpy_backend.numpy_backend.shape, other instance in _generated/pyhf.tensor.numpy_backend.numpy_backend, use :noindex: for one of them
looking for now-outdated files... none found
WARNING: A mocked object is detected: 'pytorch_backend.array_subtype'
WARNING: A mocked object is detected: 'pytorch_backend.array_type'
WARNING: A mocked object is detected: 'tensorflow_backend.array_subtype'
WARNING: A mocked object is detected: 'tensorflow_backend.array_type'

so figuring out why it thinks so is relevant.

@kratsg kratsg force-pushed the feat/jsonschema_tensorValidation branch from 9bf082f to 737422d Compare March 24, 2022 15:28
@kratsg kratsg closed this Mar 24, 2022
@kratsg kratsg reopened this Mar 24, 2022
@kratsg kratsg closed this Mar 24, 2022
@kratsg kratsg reopened this Mar 24, 2022
@kratsg
Copy link
Contributor Author

kratsg commented Mar 24, 2022

This bug is tricky. Need a way for sphinx to document a class attribute as an attribute, and not the underlying value. I wonder how I can hide this away better, because I need access to class.attr and I don't want it to be a property object. Maybe @henryiii or @jpivarski have ideas?

the short of it is

class backend:
  # my documentation for array_type
  array_type = np.ndarray

is getting expanded in sphinx with documentation/docstrings for np.ndarray and treated like a class/function, instead of treating array_type as an attribute, e.g. via something like

class backend:

  @property
  def array_type(self):
  """my documentation for array_type"""
    return np.ndarray

which is fine, but feels pretty annoying to wrap around to get access to it at the class level (backend.fget(None)) . Is there anything better? Maybe I need to mimic a @classproperty like django has.

@henryiii
Copy link
Member

Can you add it to :exclude-members:?

@kratsg
Copy link
Contributor Author

kratsg commented Mar 24, 2022

Can you add it to :exclude-members:?

Not a bad idea. I would prefer to document it as a property.

@kratsg
Copy link
Contributor Author

kratsg commented Mar 24, 2022

To be clear, by assigning a class variable to a function, sphinx is now treating it as a function, rather than a member variable. At least it shows up under the object's members, rather than under attributes. The only way I guess it to override sphinx's default?

@kratsg
Copy link
Contributor Author

kratsg commented Mar 25, 2022

To be clear, by assigning a class variable to a function, sphinx is now treating it as a function, rather than a member variable. At least it shows up under the object's members, rather than under attributes. The only way I guess it to override sphinx's default?

Filed an issue for now: sphinx-doc/sphinx#10293 with a hacky-ish workaround.

@matthewfeickert matthewfeickert added the docs Documentation related label Sep 9, 2022
@matthewfeickert
Copy link
Member

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Just stepped through this and LGTM. 👍 Thanks a lot for this @kratsg!

@matthewfeickert matthewfeickert merged commit 287bfae into master Sep 9, 2022
@matthewfeickert matthewfeickert deleted the feat/jsonschema_tensorValidation branch September 9, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related feat/enhancement New feature or request fix A bug fix schema and spec JSON schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend schema validation to support tensor types
3 participants