Likelihood synthetise method broken about xpsi HOT 13 CLOSED

I guess this is much easier to have this new output message than tracking down whether it was a parameter or a prior condition that caused the -inf likelihood.

from xpsi.

@thjsal , Maybe a message like this: Warning: Prior check failed. Either one parameter is not within bounds or a requirement set in CustomPrior has failed. will be more clear

from xpsi.

@thjsal The error message that we put to fix the issue is a bit misleading. In fact, if a parameter vector p fails the compactness test for example, then prior(p) will still return -inf. The error message will still print :'At least one of the parameters value is not within defined bound No synthesic data will be produced', even though all the parameters are within the strict bounds.

from xpsi.

I guess it depends what you define to be meaning of word "bounds". I would think returning minus infinity from prior(p) means that the parameter vector is outside of the prior bounds.

from xpsi.

I think @yveskini 's point is that the error message is misleading because the compactness is not part of the parameter list per se. So I could see this as being confusing for the users who has parameters (M and R) within the bounds but still this this error message.

from xpsi.

I see. Should we then change

print('At least one of the parameter values is not within the defined bounds.\n \
No synthetic data will be produced.')

to

print('At least one of the parameter values is not within the defined bounds or the multi-dimensional prior conditions (regarding e.g. compactness of the star) are not fulfilled.\n \
No synthetic data will be produced.')

or something like that?

from xpsi.

Well, tracking might be needed even after the new error message, but at least you will be then be more aware that it can be either of these (parameter hard bounds or a prior condition) that causes the problem. I can update this message.

from xpsi.

Let's update the message for now, and keep the issue open

from xpsi.

I updated the warning in pull request #188 . In order to close this issue, were you @sguillot hoping for a message that would directly tell whether or not it is caused by the hard bounds or by the CustomPrior conditions? I think currently doing that would require saving some extra information (or extra returned output) in the call function of Prior.py, to be able to separate these from what comes from the CustomPrior..... Although, the user who writes the CustomPrior might then still write the CustomPrior in a way which ruins this. So I am not sure if those changes would be worth doing.

I'm thinking that the new error message from Yves could be already fine.

from xpsi.

It might be actually impossible to separate these since in CustomPrior.py one can always overwrite the call function of Prior.py in whatever way that is wanted.

from xpsi.

Yes, I was arguing that a message that could differentiate Out of hard bounds VS CustomPrior conditions would be preferable for the user. But I understand if that's not easy (or impossible).

The only solution that I can think of is to compare each parameter with its bounds in the case that if not _np.isfinite(logprior). But:

1. It will slow down the likelihood calls
2. I'm not sure the bounds are accessible by a likelihood object. (are the bounds available in ParameterSubspace ?)

Please close the issue if there is no easy solution.

from xpsi.

Actually, slowing likelihood calls is not a problem, since we talk about synthesise function that is only used for generating synthetic data and not at all during sampling, as far as I understand. ...

Anyway, I realized your suggestion is possible to do and thus updated the pull request. I tested that it works using the Synthetic_data.ipynb notebook in the examples_fast directory. Please let me know, if that looks ok to you.

However, I noticed that in order for any prior checks to take even place here, I had to supply the prior object when creating the likelihood object there. If you forget to provide the prior object (like currently in our example notebook), you can still create synthetic data that is outside of your prior limits (since presumably you then have no other than the default priors, which will complain anyway if being outside of those). Was the idea to have the prior check (not using only the default strict bounds) done also in this example @yveskini ?

from xpsi.

The problem was fixed in this commit: 4f09d2e.
The synthetic data example script was there also updated to take the CustomPrior into account.

from xpsi.