Validate input line widths; scale widths based on number of leaves in the tree #275
Conversation
Of course, still need to handle case where user is obstinate and types in "-1" or whatever manually anyway...
instead of 2s
Since we're moving to having line width be explicitly in understandable units of "extra width around the line," 0 will be the baseline
Closes biocore#135! It would be nice to have some Empress / side-panel / animator tests that verify things work properly, but i think this is ok for now. Also of note: now, the default line width is 0. This will be important for biocore#144.
- Use step="any" for the line width inputs, so that l.w. isn't restricted to just integers - Change "Line Width" -> "Extra Line Width" in the UI - Make the side panel reset functions set the line widths back to 0, not 1 there's still a weird thing where switching the layout makes the new layout have thick lines, even if no thickening was previously done. huh.
If thickening *was* done previously, but wasn't currently being done, then Empress._currentLineWidth wasn't being updated to 0. This meant that, when redrawing the tree after updating the layout, an old line width was being used. Now things are good! yay. (This is the sort of case we should probs add a test for. tagging biocore#142 accordingly.)
These, plus tests, should be sufficient for us to close that issue.
Not suuper satisifed with how the scaled line widths look, or the fact that I had to make the coefficient 1 / ten million to get things to not look janky. wondering if there's a better way to do this... but this at least works. Still need to add tests, but once that's done biocore#144 should be closeable
Still doesn't consider the c0 coordinates. Is that a problem? No, I don't think so, since for any node with a c1 coordinate of (x1, y1) and a c0 coordinate of (x0, y0), the c0 coordinate will be "further in" towards the center of the circular layout at (0, 0) than the c1 coordinate -- but there will also be a parent node with a c1 also "further in", and the root's c0 = c1 at (0, 0) anyway. Thinking about this kinda makes my head hurt tbqh
(since that's what the actual empress.js uses)
Relates to biocore#184. Mostly just want to have this all documented so I don't forget it in five seconds.
I think there is still more work that should be done on biocore#144, but this is probably sufficient for a PR for now.
still in history for this branch in case it's needed
cuts down on code duplication AND i had a reason to bust out demorgan's law(s) after like five years! nice.
I guess throwing an error counts as "doing something", ... so needed to fix that
There was a problem hiding this comment.
@fedarko this looks good! I just have a couple minor comments
Co-authored-by: kwcantrell <[email protected]>
Co-authored-by: kwcantrell <[email protected]>
This updates QUnit from v1.12.0 to v1.23.1. There are later versions of QUnit, but they're of version 2.x and higher which seems to be incompatible with the way we're using QUnit (I think the main difference is that QUnit 1.x just injects stuff like "module", "ok", etc. into the global namespace, while QUnit 2.x keeps this stuff bound -- e.g. "QUnit.module", etc.). It'd be nice to update QUnit to the absolute latest versions available but for now the latest 1.x version is a decent compromise. It at least lets us use "notOk" in tests. Also, jQuery was previously stored with normal support files vendor code, but wasn't being used there (it's only in use with the tests). To make this clearer, I've moved the jQuery code to the test vendor files (alongside the QUnit stuff). For reference, the jQuery version we're on -- 2.2.4 -- seems to be the latest version of the 2.x series, so it seems at least kinda up- to-date as well.
Helps clarify a corner-case outlined by @kwcantrell on biocore#275
instead, just sort the arrays and compare those but sets should work now anyway
|
Ok, changes should be in now (now including those tests for isValidNumber() on the "1/3" case). Thanks all! |
Co-authored-by: kwcantrell <[email protected]>
| @@ -562,7 +550,7 @@ require(["jquery", "BiomTable"], function ($, BiomTable) { | |||
| deepEqual( | |||
| obsReturned, | |||
| { | |||
| 3: new Set([["o1", "o3", "o5", "o6", "o7", "o9"]]), | |||
| 3: new Set(["o1", "o3", "o5", "o6", "o7", "o9"]), | |||
There was a problem hiding this comment.
I just wanted to verify that the update version of QUnit can compare Sets. I noticed you converted the tests in getObs from Sets to Arrays but didn't do that here.
There was a problem hiding this comment.
Ah, yeah, good question. From what I can tell the new version of QUnit can compare Sets properly -- if you alter the getObsBy() tests to still use an Object of Sets (but have the Sets using the correct feature IDs), the test still works.
The reason I changed those tests to use arrays was because that was before I realized what the problem was, and wanted to see if using arrays would fix things. (If you'd like things to be more consistent here, I can either update the rest of these tests to use sorted arrays or update the getObsBy() tests to use sets; either would work.)
The reason this particular line has the inner brackets removed is because the initial way of writing it (new Set([["o1", "o3", "o5", "o6", "o7", "o9"]])) creates a Set with a single element, which is that inner array. This differs from the actual expected output, which is a Set containing each of those strings as top-level elements if that makes sense. (The old version of QUnit didn't consider this to be a problem since it couldn't compare sets, but the updated version of QUnit failed on this test. Which ... was a good thing :)
There was a problem hiding this comment.
Would you be able to revert getObsBy back to using Sets then? That way it is more clear since it will match the output of getObsBy
There was a problem hiding this comment.
Just converted it back to using sets. Thanks for the feedback :)
Addresses comment from @kwcantrell
|
thanks @fedarko! |
Closes #135. This sort of addresses #144, but I'm not suuuper satisfied with my solution to that here so I think it might be best to leave that open for now.
Work on #135 (possible to set negative line widths in the UI)
Adds
util.parseAndValidateLineWidth(), which takes as input a HTML element and checks to see if it represents a "valid" line width or not. If not, it changes the input's value to 0; in any case, it returns a float representation of the input's (new?) value. This function is used to process all 3 line width inputs in the application (sample metadata, feature metadata, animation).Work on #144 (scaling line widths to look more consistent across differently-sized trees)
I tried a bunch of different things -- at first I had a function that computed the area of the visualization and then scaled line widths based on that (see comments in #144 for more info on this plan; a version of the repo that does this is here). However, the thing with this is that the layouts (currently on the python side of things) all scale the coordinates to fit within ~4020 x 4020 ... so all of the trees I tried had roughly the same "area," as far as the JS code could tell, meaning that this didn't really do much (line widths were still kind of inconsistent between mid- and small- sized trees). I also tried @kwcantrell's suggestion of mimicking how
zoomAmountis set in empress.js and using that as a scaling factor, but this didn't seem to make that much of a difference when I tried it.I think it should be possible to either take the scaling factors for each layout in python and pass that information to the JS (or take the area of the unscaled layouts and pass that to the JS), but for a variety of reasons* I settled on doing things in a simpler way -- by just scaling line widths using the number of leaves in the tree. The formula I settled on (as of right now) is
(2 * lw) / Math.pow(Math.log10(this._tree.numleaves()), 2)Where
lwis the input line width.This seems to perform decently well for multiple sizes of trees. It's not perfectly consistent, but things seem to at least be in similar ballparks. Here are two datasets with the same set line width of 2, one with 5 leaves (a test dataset I set up, LMK if you want me to put it on dropbox or something) and one with 770 leaves (just the moving pictures tree zoomed in on a clade) --
Anyway -- as mentioned there are definitely better ways to handle this, but for now this is hopefully sufficient.
* including but not limited to: I wasn't sure how to handle things when there were different x and y scaling factors (i.e. in the rectangular layout); I didn't think of computing the area of the unscaled layout until I started writing this PR message; eventually layouts will be computed in the JS anyway; and at this point I've spent enough time wrestling with this issue that I think everyone, me included, would prefer for me to get back to working on barplots :)
Other fixes
In addition to the above stuff, this PR changes the default line widths to 0 and renames
Line WidthtoExtra Line Widthin the UI. This lets us scale line width in a more easily understandable way. I also addedstep="any"to the line width inputs, so now they shouldn't complain when you enter in float widths (e.g. 0.5). There are also a few other misc. grammar / code fixes throughout this PR (e.g. fixed a bug in the toast function and increased the default duration; abstracted out some reused code in util.js; etc.), which will hopefully make things slightly nicer.Thanks!