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

fix up indexing of summed_mask in shapesys, staterror #333

Merged
merged 2 commits into from
Oct 21, 2018

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Oct 21, 2018

Description

The main difficulty with shapesys/staterror as it affects multiple (non-consecutive) bins is the way we do masking. For situations where we need to figure out whether to enable the mask or not, it was filling in -1 when the parameter index was 0 which is not good. Additionally, tensorflow.gather() is not happy with -1.

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

@kratsg kratsg added the feat/enhancement New feature or request label Oct 21, 2018
@kratsg kratsg self-assigned this Oct 21, 2018
@kratsg kratsg requested a review from lukasheinrich October 21, 2018 17:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.714% when pulling eb6788f on fix/indexingCombinedMods into ea52590 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.714% when pulling eb6788f on fix/indexingCombinedMods into ea52590 on master.

pyhf/modifiers/staterror.py Outdated Show resolved Hide resolved
@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

@matthewfeickert done ✅

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! :shipit:

@kratsg kratsg merged commit 0fdfcc3 into master Oct 21, 2018
@kratsg kratsg deleted the fix/indexingCombinedMods branch October 21, 2018 18:57
@lukasheinrich
Copy link
Contributor

lukasheinrich commented Oct 21, 2018

Hi, the -1 was the right index in order to pick up the default value which was appended at the end of the pars in apply, do i‘m not sure this is the right change

(Sorry, On phone)

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

Hi, the -1 was the right index in order to pick up the default value which was appended at the end of the pars in apply, do i‘m not sure this is the right change

This broke in tensorflow. gather(....) kept crashing since it can't access the index. Do you want to change -1 to len(inds) - 1?

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

We also additionally changed shapesys in refactor/megachannel the same way to fix it so I just applied the same thing there to here.

@lukasheinrich
Copy link
Contributor

TF can‘t use negative indices?

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

Nope. It's gather() complains about the -1.

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

@lukasheinrich
Copy link
Contributor

Yeah then I guess it should be len-1

@kratsg kratsg added the bug Something isn't working label Oct 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants