Skip to content
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

Refactor and test legend code; show barplot legends #340

Merged
merged 60 commits into from
Aug 27, 2020

Conversation

fedarko
Copy link
Collaborator

@fedarko fedarko commented Aug 20, 2020

Closes #299 and closes #331. (Also counts as progress towards #142, since now legend.js is tested.) Also updates the barplots section of the README accordingly.

This PR adds a pretty large amount of documentation / tests, both to new and previously-existing code.

barplotlegendswoot

Various notes

  • This fixes a weird bug where it was possible for barplot layers' HTML elements' IDs to collide in certain circumstances. See this comment for an explanation.
  • The CSS changed a decent amount in this PR, but not that much -- the main reason the CSS file has a lot of diffs is I went ahead and added it to the collection of files run through prettier (so now it should be consistently styled).
  • For now, gradients match Emperor in that they go from high -> low. It would be nice to flip these around if/when Flip "Continuous values" coloring gradient emperor#782 is addressed. (For now, I assume consistency is important. also i don't know enough SVG to do this myself right now lol)

fedarko added 30 commits August 14, 2020 17:38
(need to actually make this useful, but it's a start)
This rips out "node/tip/clade key" legend stuff. (The SVG exporting
code might need updating, not sure.)

This makes the code simpler, and should make doing biocore#299 easier
Closes biocore#331.

this could still use some work, both in terms of cleaning up the code
and in making long horizontal legend items cause the legend to scroll
horizontally. i can tell it's the gradient-bar display which is
breaking that -- either flex or grid seems to explode things
turns out inline block works.

on reflection, using a <table> instead might be preferable --
that way the row structure is explicit, and we could freeze the
color header column on horizontal scrolling.
allows frozen columns! todo prettify and abstract the code...
The UI is very unpolished, but this does close biocore#299.
Super messy and kinda ugly, but hardest part is over. here on will
just be cleaning up the code / styling.
While I was throwing things at the wall to try to get the SVG
to work, i thought that part of the secret sauce in Emperor's code
might be its use of $() in creating / adding HTML elements. maybe
it was, but in any case things work now.
in this case, the continuous values checkbox is hidden but it's still
technically "checked". previously Colorer handled this case, but
now some of that duty falls to the BarplotLayer class. We coould
delegate it again to Colorer, but for now it's ok if untested and
a bit kludgy
This fixes two silly things:

1. after removing a layer, although below layers' nums changed, their
   HTML elements' IDs did *not* change. This caused problems, because
   it was possible to make a new layer with an old num and thus have
   its HTML elements' IDs match a current layer's elements' IDs, which
   messed up things like for="" attributes. Using a consistently
   unique number for the IDs fixes these problems.

2. When more than one gradient was present on the page, the oldest one
   would "override" any newer ones -- they'd have the same colors,
   even if the selected colormap was different. This was due to the
   gradient IDs all being the same. Fixing this (again using
   uniqueNum) solved the problem.
Also added a section on continuous values in the barplot legend
@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

@ElDeveloper ElDeveloper requested a review from kwcantrell August 21, 2020 17:37
Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good - thanks so much! With this refactoring in place, is there any way to easily support continuous coloring for sample/feature metadata?

README.md Show resolved Hide resolved
tests/index.html Outdated Show resolved Hide resolved
empress/support_files/js/barplot-layer.js Outdated Show resolved Hide resolved
empress/support_files/js/barplot-layer.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fedarko
Copy link
Collaborator Author

fedarko commented Aug 21, 2020

Thanks @ElDeveloper!

With this refactoring in place, is there any way to easily support continuous coloring for sample/feature metadata?

It should technically be possible, but I'm not sure it would be useful now for anything except feature metadata coloring without propagation (a.k.a. with the "Only use tip-level metadata?" checkbox unchecked). I think all of the stuff with calculating unique tips / branches for sample / feature metadata doesn't really (at least to me) make sense with quantitative data. (I guess you could have categorical groups where each group has its own number, hence the desire to color the groups by their numeric value, ...but that situation seems very unlikely to me.)

I guess it would be possible to extend our current tree coloring functionality to work better with continuous metadata, though (I know ggtree has a lot of fancy support for this, e.g. this section of their docs. One option (that could be useful for things like Songbird differentials) could maybe be adding a new feature metadata coloring method where all tips have some continuous value, and then internal nodes are assigned the average (or median, etc.) of their children's values, and so on -- and then colors are assigned, creating a sort of gradient descending from the root of the tree. This could be kinda neat, but IDK if it'd be useful or not (@mortonjt, do you have any thoughts? :)

@ElDeveloper
Copy link
Member

@fedarko fair enough that makes sense.

Copy link
Collaborator

@kwcantrell kwcantrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fedarko! The legends look really nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep legend color boxes a constant size, regardless of label text length Draw legends for each barplot layer
4 participants