search

sblunt / orbitize

Orbit-fitting for directly imaged objects
https://orbitize.info
Other
79 stars 44 forks source link

Multiplebasis #249

Closed TirthDS closed 3 years ago

TirthDS commented 3 years ago

Contains API for multiple basis sets (assigning priors and making conversions).

sblunt commented 3 years ago

Looks really good so far. I really like the simplicity of the Basis classes. I think we should discuss how we want to move ahead with treating nuisance parameters like the RV instrumental params, Hip IAD params, etc. I like the way you've done it now. Are there other nuisance params we expect to need to take into account in the future? Will this way of passing them in as kwargs to the Basis object become untenable at some point? Not sure, just starting the conversation!

semaphoreP commented 3 years ago

Yeah looks great! Nice work abstracting away all of the basis stuff into its own class. I think regarding instruments, in the future, we might want to include uncertainty in direct imaging instrument calibrations (so not just RV instruments). But I think it's ok to add additional nuisance parameters in the future as optional arguments, as long as we program it to assume that there may be additional optional parameters in the future.

sblunt commented 3 years ago

Cool, sounds good.

TirthDS commented 3 years ago

As a note, XYZ conversions on a very limited set of parameters don't seem to work even after filtering out orbits with ecc >= 1. There is always about 1-2 orbits whose parameters do not match up with the conversions.

semaphoreP commented 3 years ago

Hey @TirthDS, I see a rouge import pdb in the code, but otherwise, it's looking great. Are we ready to merge soon, or should we discuss more?

TirthDS commented 3 years ago

Hi, we should be ready to merge soon, but I am still a little concerned about the correctness of the XYZ conversions after running the tests and was hoping to discuss with @rferrerc about it.