Skip to content

Conversation

@shanikagalaudage
Copy link
Contributor

Adds p_astro likelihood as described in Eq. (40) of https://arxiv.org/abs/1912.09708

@ColmTalbot
Copy link
Owner

@shanikagalaudage, thanks for adding this! I have a couple of comments/suggestions.

I kind of like the fiducial_vt argument, although would you mind renaming it fiducial_selection? This will be more in line with the selection_function arguments. Also, please can you move it to just before cupy=True. Ideally, people would use kwargs, but that isn't always the case.

Both the comment strings should make it clear that the sampling prior distribution is the fiducial model being used.

I think it's worth adding the following to the init and removing the helper method you added.

        self._log_fiducial_selection = xp.log(fiducial_selection)

If we add this attribute to the likelihood, we should use it. I would suggest modifying _get_selection_factor in the HyperparameterLikelihood to use VT(Lambda) / VT(fiducial) rather than just VT(Lambda). I think we should leave the rate version as is, as that has a physical meaning.

Similarly, unless there's a reason not to, I think PastroLikelihood._get_selection_factor should use the same definition, it'll make the code a bit neater.

I would also rather the default be fiducial_selection=1, rather than the current 0.005. This is also an arbitrary number, but less confusing to see hard-coded in. Finally on this argument, can you remove the log message from the HyperparameterLikelihood, although I think it should be a warning in the PastroLikelihood. In the same vein, maybe we should add a warning if self.data['prior'] is 1 everywhere for the new likelihood.

Also, please remove all reference to pastro from the main likelihood class. This will require adding an __init__ method to the new likelihood class.

Sorry for the long comment...

@shanikagalaudage
Copy link
Contributor Author

@ColmTalbot thanks! I will implement these changes.

Similarly, unless there's a reason not to, I think PastroLikelihood._get_selection_factor should use the same definition, it'll make the code a bit neater.

There is a difference being the _get_selection_factor for the general likelihood is multiplied by the number of posteriors and PastroLikelihood._get_selection_factor. Changing the function to be more general and omit the number of posteriors would make the code cleaner.

This will require adding an __init__ method to the new likelihood class.

This would involve just copying and pasting what I currently have in the init function to the PastroLikelihood, correct?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants