-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix radec/seppa conversion with OFTI sampler #109
Conversation
…m.data_table` upon initialization.
…c to sep/PA for a given object number. Updates `data_table` in place and updates the `system.radec` and `system.seppa` arrays which track which rows are in which format
…TI` (only exists in `System` now). Uses new method from `System` to do radec/seppa conversion that will properly update `data_table` Copying observed astrometry from `data_table` now happens *after* radec/seppa conversion.
…ged to system object
Pull Request Test Coverage Report for Build 730
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. convert_data_table_radec2seppa
is a pretty long function name but I can't think of anything shorter.
Looks good to me! Thanks again @henry-ngo! I agree that the function name is long but I like that it's descriptive. |
I think this is definitely an improvement, thanks Henry! |
This is a fix for Issue #108
As discussed, I remove
sampler.data_table
andsampler.input_table
in favour ofsystem.data_table
andsystem.input_table
. I moved the actual radec/seppa conversion calculation to be done within thesystem
object, as I wanted it to changesystem.data_table
in place while also updating the arrayssystem.radec
andsystem.seppa
used to keep track of which entries are in which format. I believe these arrays are necessary in additional to thequant_type
column ofdata_table
because it sorts them by body number.Right now, these conversions are done for one body at a time since we don't do multi-planet yet. It seems like the OFTI sampler was already hard-coded for just 1 planet so I kept it that way to keep the scope of this PR only on #108. It can be made to accommodate multiple planets when we make that bigger update!