-
Notifications
You must be signed in to change notification settings - Fork 16
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
Labels fix #304
Labels fix #304
Conversation
return ac([(_LocIndexer_("loc", | ||
super(_LabelledObject, | ||
self.obj.drop_labels(i)) | ||
).__getitem__, | ||
self.obj.get_labels_map(i)) | ||
for i in self.obj._all_axes()], key) |
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.
There are several changes to all of the indexers/accessors:
_all_axes
now returnsNone
as well, to capture the extra edge case (which was previously tagged on as+[...]
- crucially we return the
super
object in theIndexers
(this is one of those, 'how did that ever work?' observations) - We return a list of (function, labels_map) pairs, rather than just a list of functions.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #304 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 2667 2691 +24
=========================================
+ Hits 2667 2691 +24
☔ View full report in Codecov by Sentry. |
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.
A few comments inline and two general points below.
Following offline conversations with @lukashergt and @AdamOrmondroyd, this PR aims to resolve #253 and #256.
- I believe the reference to replace
.boxplot()
with.plot.box()
#256 was actually supposed to be Newpandas.DataFrame.attrs
feature possibly useful for metadata such asroot
orlabel
#236, right? - This PR does not address the incorrect (non tex) axes labels from Retain latex label when slicing DataFrame to Series #253 in:
Do we want this PR to address that issue as well?
samples.plot.kde_2d('x0', 'x1')
Note that the other part of Retain latex label when slicing DataFrame to Series #253 is resolved:samples.x0.plot.kde() samples.x1.plot.kde() plt.legend()
yes : ( |
Done in 3f1e2e6 |
x = data[self.x].values | ||
y = data[self.y].values | ||
x = data[self.x] | ||
y = data[self.y] | ||
self.x = x.name # transfer the tex label | ||
self.y = y.name # transfer the tex label |
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.
This nicely works for labelled dataframes, but now unlabelled dataframes don't have any x- and y-labels anymore:
samples[['x0', 'x1']].drop_labels().plot_2d()
See also some suggested tests below.
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.
I think this requires a similar check à la:
if x.name:
self.x = x.name
i.e. only use x.name
if it is not empty.
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.
Nevermind, that won't help...
I think we might need to ensure that samples.drop_labels().get_label('x0')
does not return an empty string, but 'x0'
instead...
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.
done in aa715ab I think no
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.
I think this requires a similar check à la:
if x.name: self.x = x.namei.e. only use
x.name
if it is not empty.
I put this in and took it back out again now that name is always used when slicing, but maybe worth retaining anyway
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.
tackled in 10a5b42
tests/test_samples.py
Outdated
def test_plot_1d_no_axes(): | ||
np.random.seed(3) | ||
ns = read_chains('./tests/example_data/pc') | ||
ns[['x0', 'x1', 'x2']].plot_1d() |
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.
Following up on my above comment, we should add something along the lines of:
axes = ns[['x0', 'x1', 'x2']].plot_1d()
assert axes[0].get_xlabel() == '$x_0$'
assert axes[1].get_xlabel() == '$x_1$'
assert axes[2].get_xlabel() == '$x_2$'
axes = ns[['x0', 'x1', 'x2']].drop_labels().plot_1d()
assert axes[0].get_xlabel() == 'x0'
assert axes[1].get_xlabel() == 'x1'
assert axes[2].get_xlabel() == 'x2'
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.
I've added these tests, but the drop_labels()
ones are failing as expected. What would you expect for a mixture of labelled and unlabelled columns?
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.
If a parameter/column has a (tex) label, use that, if a parameter/column does not have a (tex) label then use the column handle.
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.
I've fixed this for the separate plots by only changing name
to the label if the label exists, rather than setting it to a blank string as currently happens.
This solves the issues for individual plots (e.g. plot.kde_1d()
), but not plot_1d/2d()
, which use the label mapping to create the axes, which leads to blanks.
anesthetic/samples.py
Outdated
If not provided, then all parameters are plotted. This is intended | ||
for plotting a sliced array (e.g. `samples[['x0','x1]].plot_1d()`. | ||
It is not advisible to plot an entire frame, as it is | ||
computationally expensive, and liable to run into linear algebra | ||
errors for degenerate derived parameters. |
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.
Is this true for the 1d case?
Failing tests appear to be an issue in fastkde, not in anesthetic :/ |
For this example one could claim that in addition to creating a new parameter |
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.
Happy with the change in 10a5b42 ?
Opinions on #304 (comment) ?
Otherwise happy to approve this, now.
Yes -- to my mind this is quite a neat API.
Adjusted -- will need reapproval |
Although in an ideal world this would have '$x_0$' 'a' and 'b' as labels... |
I think I side with Lukas - not only is this PR then done, but blank axes serve as a reminder to the user to |
Description
Following offline conversations with @lukashergt and @AdamOrmondroyd, this PR aims to resolve #253 and #236.
It upgrades labelled data frames so the
LabelledSeries.name
attribute now reads as the tex, rather than the label (or something else), which will prove useful in improving plotting tools.It achieves this by implementing the strategy "If the resulting accessed slice has a 'name' attribute, set this to the value of the label that was dropped".
e.g. a frame with a multiindex column element
('vowel','A','$A$')
, would returnframe.vowel.A
as a series with the name'$A$'
, rather than'A'
or('vowel','A')
.The tests have been updated to reflect this new behaviour, and new tests added to check that this occurs.
Checklist:
flake8 anesthetic tests
)pydocstyle --convention=numpy anesthetic
)python -m pytest
)