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

feat: Add support for arrayful JSON #1647

Merged
merged 13 commits into from
Oct 18, 2021
Merged

feat: Add support for arrayful JSON #1647

merged 13 commits into from
Oct 18, 2021

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Oct 15, 2021 •
edited by matthewfeickert
Loading

Description

this is a step towards #882 and allows tensor-like values in the spec

we relied a lot on += being concatenatoin for python lists.. but now we first collect a list of lists/arrays and then do tb.concatenate

@phinate

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
* Allow for tensor-like values in model spec by building the
modifier builder_data from a list of lists/arrays and then
using tensorlib.concatenate to built up the sample properties.
* Add test for building a model with tensor inputs.

@matthewfeickert matthewfeickert marked this pull request as draft October 15, 2021 16:11
@lukasheinrich lukasheinrich changed the title arrayful json feat: arrayful json Oct 15, 2021
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Oct 16, 2021
@codecov
Copy link

codecov bot commented Oct 17, 2021 •
edited
Loading

Codecov Report

Merging #1647 (16393e1) into master (fb3a727) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1647   +/-   ##
=======================================
  Coverage   98.03%   98.04%           
=======================================
  Files          63       63           
  Lines        4129     4145   +16     
  Branches      566      572    +6     
=======================================
+ Hits         4048     4064   +16     
  Misses         48       48           
  Partials       33       33
Flag Coverage Δ
contrib 24.89% <3.44%> (-0.10%) ⬇️
unittests 97.82% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/modifiers/histosys.py 100.00% <100.00%> (ø)
src/pyhf/modifiers/shapesys.py 100.00% <100.00%> (ø)
src/pyhf/modifiers/staterror.py 97.59% <100.00%> (+0.15%) ⬆️
src/pyhf/pdf.py 97.70% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb3a727...16393e1. Read the comment docs.

@lukasheinrich lukasheinrich marked this pull request as ready for review October 17, 2021 10:30
kratsg
kratsg requested changes Oct 17, 2021
View reviewed changes
Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

we need tests?

@lukasheinrich
Copy link
Contributor Author

added!

@lukasheinrich
Copy link
Contributor Author

Should be good to go @kratsg @matthewfeickert

@matthewfeickert matthewfeickert changed the title feat: arrayful json feat: Add support for arrayful JSON Oct 18, 2021
@matthewfeickert matthewfeickert added the tests pytest label Oct 18, 2021
matthewfeickert
matthewfeickert approved these changes Oct 18, 2021
View reviewed changes
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.

LGTM. 👍 Thanks!

@matthewfeickert
Copy link
Member

cc @ponyisi as follow up to this Twitter thread. :) This will be in v0.7.0 (the next release).

@matthewfeickert
Copy link
Member

matthewfeickert commented Oct 18, 2021 •
edited
Loading

I think the CI is hanging because of Issue #1486 unfortunately. I'm happy to merge this in even with if "failing" as it was passing before (but I'm also a person who gets an irrational level of reassurance from seeing everything green so I'm happy to poke it a few more times).

@matthewfeickert
Correct naming
3eacd1e 
sample -> modifier
sample_property -> sample
@matthewfeickert
Copy link
Member

Oh, I think it is actually Issue #1451. Testing and if so then fixing it now.

@matthewfeickert matthewfeickert mentioned this pull request Oct 18, 2021
Merged
4 tasks
@matthewfeickert matthewfeickert merged commit be5ae65 into master Oct 18, 2021
@matthewfeickert matthewfeickert deleted the arrayful_json branch October 18, 2021 21:04
@matthewfeickert
Copy link
Member

Thanks @lukasheinrich. 👍

@ponyisi
Copy link

ponyisi commented Oct 18, 2021

Thanks for taking care of this!

@phinate phinate mentioned this pull request Oct 19, 2021
Merged
4 tasks
@Moelf Moelf mentioned this pull request Oct 22, 2021
Open
@matthewfeickert matthewfeickert mentioned this pull request Dec 7, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewfeickert matthewfeickert matthewfeickert approved these changes

@kratsg kratsg kratsg approved these changes

Assignees

@lukasheinrich lukasheinrich

@matthewfeickert matthewfeickert

Labels
feat/enhancement New feature or request tests pytest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukasheinrich @matthewfeickert @ponyisi @kratsg