Photometric Plot Simplification #176
Conversation
Will-Cooper
added
enhancement
|
Something I noticed while testing this: if you change up what the CMD are plotting and then click Bokeh's reset view button- you can end up in a situation where no data is shown unless you zoom out. What I think is happening is that Bokeh's reset view is acting on the original data and so the axis limits are saved there for purposes of resetting. Changing what you are plotting does not update this and so you end up having to zoom out and re-find your data if you click that button. I had expected that to be more of a zoom to show all data rather than go back to what was first plotted. Not something to be fixed in this PR, but something we may want to consider. |
| @@ -758,6 +761,9 @@ def pogson_law(m: Union[float, pd.Series]) -> Union[float, np.ndarray]: | |||
| _abs_mag[mask] = m[mask] + 5 * np.log10(df.parallax[mask]) - 10 | |||
There was a problem hiding this comment.
Is there a reason why pogson_law is defined inside another function? I know python allows for this, but for better testing, reuse, and clarity it could be moved outside of it.
There was a problem hiding this comment.
no real reason other than it's the only place it was needed, and I find it fun taking advantage of encapsulation. I do this in a few places IIRC
| @@ -190,6 +190,23 @@ <h2 class="display-4">Companion Relationships</h2> | |||
| </p> | |||
| {% endif %} | |||
| </div> | |||
|
|
|||
| <div class="row table-responsive"> | |||
There was a problem hiding this comment.
Separate from PR topic, but I was able to confirm this part is working fine.
There was a problem hiding this comment.
ah apologies, this was for fixing #166
| @@ -76,6 +76,24 @@ def bokeh_formatter(p: figure) -> figure: | |||
| return p | |||
|
|
|||
|
|
|||
| def name_simplifier(s: str) -> str: | |||
There was a problem hiding this comment.
Not sure if you have a test for plot-related functions, but this one would be easy to write tests for.
There was a problem hiding this comment.
will add this to #145
|
Approving since it looks good overall. We may want to create a few issues for some of the other comments to address them separately. |
yes, I've noticed this myself and I think the cause is as you say. It's worth raising a minor issue because I think I have created a workaround for the default behaviour before. |
Making the CMDs only use G, J and W1 and colours only G-RP, G-J, J-K, H-K, W1-W2.
Also some minor fixes and getting started on some documentation changes. Trimming zeroes from flux arrays in spectra which was throwing off certain spectra. Add modeled parameters table to solo results page.