brinckmann / montepython_public

Public repository for the Monte Python Code
MIT License
93 stars 77 forks source link

Bounded parameters of -1.0 are ignored! #323

Open ThomasTram opened 1 year ago

ThomasTram commented 1 year ago

Hi there

The commit c692dee substituted the identity comparison operator is for the equality comparison operator ==:

https://github.com/brinckmann/montepython_public/blob/4f010ad7ede7306ada811de80cad8369dbd6338f/montepython/prior.py#L56-L57

However, this means that, contrary to statements in the parameter files, -1.0 can no longer be used as a prior bound! The reason it worked before was that, in CPython, the integers -5 to 256 are pre-allocated, so every integer in this range are in fact the exact same object. MontePython was relying on this to ensure that only -1 was set to None, and not -1.0.

A few comments are in order:

While the original implementation would still work, it will emit a syntax warning since Python 3.8, so it is not a viable solution. My preferred solution would be to remove the "feature" that -1 may signal unbounded parameters. However, the following change should reestablish the intended behaviour:

        self.prior_range = [a if not((a == -1 and type(a) == int) or (a == None)) else None
                            for a in deepcopy(array[1:3])]

Let me know if you want a PR for this!

Cheers, Thomas

brinckmann commented 1 year ago

Hi Thomas,

Thank you for noticing and reporting this. I've never liked that option either, it doesn't make sense. We should stop promoting it. But for the sake of people who just keep doing what they've been doing something like your fix could be a good idea, or maybe returning a warning or error if we see they have passed -1. I'm also worried about cases where people actually want a prior bound at -1 and might not think to make it a float, so returning a statement like "Noticed prior bound of -1 in param file. If no prior bound is intended please pass None, if an actual prior bound of -1.0 is intended please pass a float." What do you think? Or we can ignore the problem and go with your suggestion, which seems workable.

Best, Thejs