-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[ENH] Scatter Plot: Improve tooltips #2703
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2703 +/- ##
==========================================
+ Coverage 76.16% 76.21% +0.05%
==========================================
Files 338 338
Lines 59879 59925 +46
==========================================
+ Hits 45606 45674 +68
+ Misses 14273 14251 -22 |
text += ' {} = {}\n'.format(v, self.data[index][v]) | ||
if self.tooltip_shows_all: | ||
for columns in (CLASS, META, ATTRIBUTE): | ||
text = show_it(text, 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.
Unpack: *columns
.
META = ("Meta", "Metas", 4, domain.metas) | ||
ATTRIBUTE = ("Attribute", "Attributes", 10, domain.attributes) | ||
|
||
def show_it(text, 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.
Take columns
spec unpacked.
var = cols[columns[2]] | ||
text += "".join(' {} = {}\n'.format(var.name, self.data[index][var])) | ||
elif n_cols > columns[2]: | ||
text += " ... and {} others\n\n".format(n_cols - columns[2]) |
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 code is an insult to this craft.
HTH
I can't merge this, it's mostly my code. Can someone else take a look, please? |
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.
When the data set has a class variable and that is chosen for e.g. Axis x, the tooltip has an empty class section. This is/looks incorrect.
I see two options:
- print the class again in the class section even if it was already listed in axis x or y
- omit the class section
Number 2) seems more concise, but becomes problematic for multiple classes (e.g. one of two classes is Axis-x, printing just the other one in the Class section makes it look like the example has just one class variable)
Anyway, anything is better than to display an empty class section for an instance with a defined class.
Metas have the same problem as class and need to be handled the same way.
for var in (self.shown_x, self.shown_y)) | ||
if self.tooltip_shows_all: | ||
text += "<br/><br/>" + \ | ||
"<br/>".join(show_part(point_data, *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.
I suggest joining with "" here and removing one <br/>
in the above line. And instead adding the <br/>
at the beginning of line 1199.
Currently, if show_part
returns an empty string it is still joined with the rest using <br/>
, producing superfluous blank lines.
The empty class section was fixed, but metas were not! |
Done. I made a separate commit, so you can see the change -- and also so that I can revert it if that's not what we'd like. Class is treated separately (not removed even if used for axes), while metas and attributes are removed when used. When you confirm it's OK, I'll squash the commits. |
Looks good! Merging after squash... |
Squashed. |
Description of changes
Includes