California-Planet-Search / radvel

General Toolkit for Modeling Radial Velocity Data
http://radvel.readthedocs.io
MIT License
57 stars 52 forks source link

Using a list in EccentricityPrior raises TypeError #347

Closed vandalt closed 3 years ago

vandalt commented 3 years ago

Hi, When using a list to initialize EccentricityPrior, I get the following error:

----> 1 radvel.prior.EccentricityPrior([1, 2])

~/astro/radvel/radvel/prior.py in __init__(self, num_planets, upperlims)
     96
     97         if type(upperlims) == float:
---> 98             self.upperlims = [upperlims] * npl
     99         else:
    100             assert len(upperlims) == len(self.planet_list), "Number of eccentricity \

TypeError: can't multiply sequence by non-int of type 'list'

This seems to happen because npl is set to num_planet when it is a list (see Line 95). I just checked and changing npl to always be len(self.planet_list) or switching lines 92 and 95 both work.

bjfultn commented 3 years ago

The calling syntax should be radvel.prior.EccentricityPrior(2, 0.90) (for example). This would put a hard upper limit at e=0.9 for all (2) planets in the system.

vandalt commented 3 years ago

Using an integer indeed works, but the documentation specifies that num_planets can be either an integer or a list. And the code currently handles both cases, but does not assign npl to an integer or a list. The integer option works fine for my current use-case, but the list option could be useful when one planet has a fixed circular orbit or has a different eccentricity prior for some reason. I'd be happy to send a PR with this modification (it's only a two-line modification), but I understand if you prefer to support only the integer syntax in the future.

bjfultn commented 3 years ago

If a list is provided for num_planets then a list of the same length must be provided for upperlims. In your original example its using the default upperlims=0.99 since no limit is given.

If you want to fix eccentricity you should use the vary=False keyword on the Parameters objects that you want to fix.

So the fix here would be to make upperlims a list with the same length as num_planets if num_planets is provided as a list and upperlims is not.

I would love it if you want to make that fix and submit a PR!

vandalt commented 3 years ago

If you want to fix eccentricity you should use the vary=False keyword on the Parameters objects that you want to fix.

Yeah sorry that wasn't clear, I meant that once it is set to vary=False, not setting a prior on it makes sense from the perspective of a user. It was just an example of a use-case where the list option is useful.

So the fix here would be to make upperlims a list with the same length as num_planets if num_planets is provided as a list and upperlims is not.

This was already implemented, the only issue was that instead of using the length of num_planets to make an upperlims list, num_planets itself was used (hence the TypeError).

See #348 for the PR!

bjfultn commented 3 years ago

Great, lets continue the discussion on the PR thread.