California-Planet-Search / radvel

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

Kitchen Sink #29

Closed petigura closed 8 years ago

petigura commented 8 years ago

We should have a serious conversation about where we draw the boundaries for radvel. There are a lot of bells and whistles in the package, including complex plotting routines and jury-rigging in mstar and rp planet. These don't enter into the core numerics basically go a long for the ride. I'm wondering if it makes sense to refactor out some of this code into a higher level "pipeline" package and a lean radial velocity-fitting package.

awhoward commented 8 years ago

Let's discuss at group meeting tomorrow. Separating fundamental radvel functionality from plotting and related analysis makes sense to me. I don't have strong feels about the dividing lines though.

petigura commented 8 years ago

Another reason to separate out the derived parameters:

If stellar masses or planet radii are absent in the input files, the mcmc can proceed to completion, but die due to lake of these parameters.

te = 48.9%; Min Tz = 48680.8; Max G-R = 1.03      
Chains are well-mixed after 90000 steps! MCMC completed in 30.6 seconds

Traceback (most recent call last):
  File "/Users/petigura/Research/radvel/bin/kepfit.py", line 182, in <module>
    scale=P.planet['rp_err{}'.format(i)],
KeyError: 'rp_err1'
petigura commented 8 years ago

As discussed previously, there's a danger that trying to get bin/kepfit.py to handle all aspects of the radvel workflow will make the code cumbersome, hard to maintain, and hard to extend. My sense Is that we like to define all the inputs to the code inside our setup files, which is fine, but it would be cleaner if we separated the analysis into well-defined subtasks, that can easily be scripted together. Here's a sketch:

# perform the initial max-likelihood fit, save plot
$ radvel fit epic205071894.py

# different directory needed... no problem
$ radvel fit epic205071894.py -o ${HOME}/

# performs mcmc, save plot, save chains
$ radvel mcmc epic205071894.py --nsteps=20000 --nburn=1000

# perform BIC model comparison with polynomial trends of different complexity
$ radvel bic trend epic205071894.py 

# perform BIC model comparison with polynomial trends of different complexity
$ radvel bic nplanets epic205071894.py 

# Multiplies mcmc chains by physical parameters.
$ radvel derive epic205071894.py

# Create latex table of RVs for publication
$ radvel table rv epic205071894.py 

 # Creates latex table of rv parameters (P, K, etc) and derived parameters (Msini, rho, etc)
$ radvel table params epic205071894.py

# Assembles the output PDFs and tables into a single latex file
$ radvel report epic205071894.py 

@bjfultn: what do you think?

bjfultn commented 8 years ago

Works for me. I will just have to write new code to keep track of (and create) input and output files from the various steps and code to check that previous steps have been run.

I think that this might belong in a separate issue thread though since it doesn't address the issue of when to stop adding features.

petigura commented 8 years ago

Thanks for implementing the new CLI, BJ. Much more modular.