Plot refactor - #415
Conversation
| cmap = colors.LinearSegmentedColormap.from_list( | ||
| "trunc({n},{a:.2f},{b:.2f})".format(n=cmap.name, a=0.0, b=0.7), | ||
| cmap(np.linspace(0.0, 0.7, 1000)), | ||
| ) |
There was a problem hiding this comment.
Do we need to define cmap here and below? Can we reduce it to just one def, either at the top here or in the Plotter class?
| def __init__( | ||
| self, | ||
| results, | ||
| object_to_plot=1, # TODO: Support multiplanet |
There was a problem hiding this comment.
Noting that we should resolve this todo before merge (I'm sure this is already on your radar).
| self.results = results | ||
| self.system = results.system | ||
|
|
||
| if default_cmap is not None: |
There was a problem hiding this comment.
Make sure the logic works here to define the colormap as we expect
| ecolor=c, | ||
| zorder=5, | ||
| ls="none", | ||
| label="{} gamma err".format(name2) |
There was a problem hiding this comment.
I'd probably suggest writing these captions out in latex to make them as clear as possible: std(rv_{inst name} - gamma_{inst name}); sigma_{inst name}2 + sigma2 (and same for the other case below)
| deoff = self.astr_deoff | ||
| seps = [] | ||
| pas = [] | ||
| raoff_100 = self.raoff1 |
There was a problem hiding this comment.
use your defined variables throughout rather than redefining them (for clarity)
|
|
||
| # for j in range(len(astr_epochs)): | ||
|
|
||
| # seps0, pas0 = orbitize.system.radec2seppa(raoff[i][j], deoff[i][j], mod180=mod180) |
| arcs with PAs that cross 360 deg during observations (default: False) | ||
|
|
||
| Return: | ||
| ``matplotlib.pyplot.Figure``: the residual plots |
There was a problem hiding this comment.
Let's use the same logic for plotting residuals as we used above for plotting the gammas for RVs. Offer two options for plotting the uncertainties, exactly the same way you did above.
| timestep = epoch_in_yr[1] - epoch_in_yr[0] | ||
| # dra/dt and ddec/dt | ||
| ddec_b = np.gradient(deoff[i, :], timestep) # in mas/yr | ||
| dec_b_radian = ( |
There was a problem hiding this comment.
let's use astropy.units to do these conversions for clarity
|
Looks great! Excellent job @eshelDror. A few small overall comments:
|
|
|
||
| if plot_priors: | ||
| axes = figure.axes | ||
| num_params = len(param_indices) |
There was a problem hiding this comment.
I think you can avoid much of this normalization code by passing hist2d_kwargs = {"density":True} into corner.corner.
| for i, param_i in enumerate(param_indices): | ||
| prior = results.system.sys_priors[param_i] | ||
| if not hasattr(prior, "compute_lnprob"): | ||
| continue |
There was a problem hiding this comment.
This should never happen; Prior objects by definition (by means of the abstract base class) must have a compute_lnprob method, so I think you can safely remove this check
|
Also, see aesthetic suggestions in #378 and please implement the ones you like! I think the main things I'd like to see here based on those suggestions are:
|
2 participants
No description provided.