California-Planet-Search / radvel

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

update planet_parameters with change of basis #321

Closed megbedell closed 4 years ago

megbedell commented 4 years ago

Make sure that the planet_parameters attribute of a Parameters instance matches the chosen basis even after transforming.

This is a fix to the following bug:

params = radvel.model.Parameters(2, basis='per tp e w k')
# set some parameter values here
synth_basis = radvel.basis.Basis(params.basis.name, params.num_planets)
mcmc_params = synth_basis.from_synth(params, 'logper tc secosw sesinw logk', keep=False)
print(mcmc_params.planet_parameters)

would formerly return the old basis (per tp e w k) instead of the updated basis (logper tc secosw sesinw logk). (At least I think that's a bug -- unless I'm misunderstanding the desired behavior?)

bjfultn commented 4 years ago

This does indeed look like a bug. Thank you for the contribution @megbedell!

bjfultn commented 4 years ago

I changed the base to next-release, can you pull/merge next-release into your branch to make sure its up to date?

megbedell commented 4 years ago

@bjfultn I did the following, is that sufficient or do I need to move the edits that I made in my fork's default branch to the next-release branch and update the PR? Sorry, I'm not the best with github!

(radvel) Megans-Work-Laptop:radvel mbedell$ git checkout next-release
Branch next-release set up to track remote branch next-release from origin.
Switched to a new branch 'next-release'
(radvel) Megans-Work-Laptop:radvel mbedell$ git status
On branch next-release
Your branch is up-to-date with 'origin/next-release'.
nothing to commit, working tree clean
bjfultn commented 4 years ago

Looks like you are good. Now we just wait for the tests to run...