Plot pl rvs - Githubissues

sblunt / orbitize

Orbit-fitting for directly imaged objects

https://orbitize.info

Other

74 stars 43 forks source link

Plot pl rvs #366

Closed sblunt closed 5 months ago

sblunt commented 5 months ago

Hey @chihchunhsu, I worked on your PR a little bit and put the changes in this branch (all the lines you changed are still attributed to you, so never fear).

Main things I changed:

pulled in all v3 changes, so now this is using the latest version of the code
added two unit tests to test that the plotting code runs with just secondary RVs and with a mix of primary and secondary.

One small issue remains: it looks like the placement of the "instruments" legend is off (see below-- this is output from the tests/test_rv_plotting:test_secondary_rv() function I just added). Would you be willing to take a look? I invited you as a collaborator, so you should be able to make changes directly to this branch.

foo

chihchunhsu commented 5 months ago

Thanks for merging these @sblunt! I wanted to check how you want this to be implemented. I think the issue now is on the plot.py line 568: plt.legend(title='Instruments', bbox_to_anchor=(1.3, 1), loc='upper right'), where I hard coded the legend to be outside of the figure. Do you want this instrument legend in the PA/Sep panels? Or do you want the instrument also for the RV panel too?

sblunt commented 5 months ago

Got it! I think the instruments panel should be within the RV panel of the figure. At this point we only label different points taken by different RV instruments by default (since we allow them to have different instrumental nuisance parameters).

chihchunhsu commented 5 months ago

I moved the legend back into the panel; actually it was the PA/Sep instrument legend being outside. Would this version work? I moved the legends to the upper right corner, but please let me know if you want to have them located somewhere else, or just let the matplotlib decide

bar foo

sblunt commented 5 months ago

That looks good to me. One of the unit tests had an unrelated failure, so I just pushed a fix.

I think we shouldn't label the sep/PA datapoints, so I'll also just remove that line of code. But otherwise this looks great. I'll pull it in once the tests pass. Thanks so much Dino!

sblunt commented 5 months ago

Oh, never mind. I see now that code is inside a conditional statement to plot the astrometry instruments. Looks great!

chihchunhsu commented 5 months ago

Great! Thanks for fixing the numpy issue for OFTI. Happy to contribute and have something useful coming out of the hackathon!