DM-40180: Remove 'all' histograms from scatter plot with two hists #137
Conversation
sr525
force-pushed
the
tickets/DM-40180
branch
4 times, most recently
from
75879bd
to
a8af0e8
Compare
sr525
force-pushed
the
tickets/DM-40180
branch
2 times, most recently
from
411a041
to
b363d11
Compare
| x_all = data[f"x{self._datatypes['any'].suffix_xy}"] | ||
| keys_notall = [x for x in self.plotTypes if x != "any"] | ||
| if "all" in self.plotTypes: | ||
| x_all = f"x{self._datatypes['all'].suffix_xy}" |
There was a problem hiding this comment.
It needs both to have the string key inside the string.
| if "all" in self.plotTypes: | ||
| x_all = f"x{self._datatypes['all'].suffix_xy}" | ||
| keys_notall = [x for x in self.plotTypes if x != "all"] | ||
| topHist.hist(x_all, bins=bins, color="grey", alpha=0.3, log=True, label=f"All ({len(x_all)})") |
There was a problem hiding this comment.
Why are you still defining an x_all below the else? Ditto for y_all below. Actually, you can probably move this setting of keys_notall above the if and just get rid of the else? Same below, again.
There was a problem hiding this comment.
The all should probably be any which brings it in line with the key used in the datatype option. It is for if people don't care if it is a star or a galaxy and just want to plot everything. I have changed the alls -> anys in a hope to make this clearer.
| @@ -711,17 +711,17 @@ def _makeSideHistogram( | |||
| ) -> None: | |||
| # Side histogram | |||
| sideHist = figure.add_subplot(gs[1:, -1], sharey=ax) | |||
There was a problem hiding this comment.
Pre-existing, but there is a rogue # Side histogram comment at line 701.
There was a problem hiding this comment.
This comment is just reminding people that they are in the chunk that defines the side histogram.
| if "any" in self.plotTypes: | ||
| x_all = data[f"x{self._datatypes['any'].suffix_xy}"] | ||
| keys_notall = [x for x in self.plotTypes if x != "any"] | ||
| if "all" in self.plotTypes: |
There was a problem hiding this comment.
I don’t know what any actually means in this context, but now the behaviour is that it considered a notall key. Does this still do the “right thing”?
There was a problem hiding this comment.
It should all be any, not sure what I was doing here.
sr525
force-pushed
the
tickets/DM-40180
branch
from
b363d11
to
176adab
Compare
No description provided.