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

allow using constraint in the config which sets vector margin params #4868

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WuShichao
Copy link
Contributor

Standard information about the request

This is a: bug fix

This change affects: inference

This change will: nothing

Motivation

If there are marginalize_vector_params in the config file and constraint sections to some variables, PyCBC will
apply those constraints to those marginalized parameters array, it will show array-shape mismatch in array error.

Contents

Not pass those marginalized parameters to self.prior_distribution.

Links to any issues or associated PRs

Testing performed

Additional notes

  • The author of this pull request confirms they will adhere to the code of conduct

@WuShichao
Copy link
Contributor Author

@ahnitz, Can this CC issue be ignored? https://codeclimate.com/github/gwastro/pycbc/pull/4868

@@ -563,7 +563,16 @@ def logprior(self):
def _logprior(self):
"""Calculates the log prior at the current parameters."""
logj = self.logjacobian
logp = self.prior_distribution(**self.current_params) + logj
# allow using constraint in the config which sets
Copy link
Member

Choose a reason for hiding this comment

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

This seems to leak the interface of the marginalization parameters so that it is coded in multiple places. Would it be better to have the DistMarg class perhaps just override this function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain more?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants