Empress' JS gets stuck in the loading animation when none of the samples in the table have any metadata #139
Comments
fedarko
added a commit
to fedarko/empress
that referenced
this issue
Mar 23, 2020
I still want to add more tests, but this should at least tentatively close biocore#139.
|
WHOOPS this shouldn't be closed, sorry I had to move my work on #139 out of the rectangular layout branch and one of those commits already closed this issue and I guess that commit message still worked its auto-closing magic. ... Will make another PR for that soon ;) |
fedarko
added a commit
to fedarko/empress
that referenced
this issue
Apr 7, 2020
Closes biocore#139, for real this time. Eventually we'll need to check that feature metadata matches up, but that is its own problem for later down the road.
fedarko
added a commit
to fedarko/empress
that referenced
this issue
Apr 8, 2020
ElDeveloper
added a commit
that referenced
this issue
Apr 20, 2020
… ok (#154) * BUG/TST: Add back in data matching/checking code Closes #139, for real this time. Eventually we'll need to check that feature metadata matches up, but that is its own problem for later down the road. * STY: fix flake8 complaint * DOC: add all needed moving pix files & "make docs" Not sure why these files weren't here before, but this will make rerunning the tutorial easy. Also "make docs" is just a shorthand that saves extra typing when re-visualizing the moving pictures tree. We could integrate this into the travis build in the future if desired (of course this would be predicated on us getting QIIME 2 set up in the travis build, which would add on a few minutes to each build due to Q2 installation taking some time). * DOC: typo fix [ci skip] * BUG: Transpose feature tbl before matching it So apparently QIIME 2's transformers from biom table -> pd DataFrame produce DFs that are transposed from what biom.Table does -- QIIME 2 uses samples as the indices (rows) and features as the columns, while biom.Table does it the other way around. As you can imagine, this is pretty confusing! This commit should fix this problem from our end, but in the future we should really add logic to prevent having to do table-DF-transposition, since IIRC that can be super slow with massive DFs. (...We really oughta unit-test _plot.) * STY: rm extra blank line * TST: rename a prev matching test and add skeletons * TST: Add "no features shared" test for matching part of #139 fixes * TST: test a warning msg printed during matching * TST: Add sample dropping warning test think this pr should be good for now * DOC: add note to match_inputs() re #130 (TODO) * TST: Install and use QIIME 2 env in travis build * TST: Add actual Q2 integration test! Addresses @ElDeveloper's comment on #154. I'm keeping 'make docs' around since it could still be nifty (if you just wanna regenerate the empress-tree.qzv file without rerunning the tests, I guess). * TST: don't run 'make docs' on travis build Since the Q2 Artifact API test I just added does the same thing. * TST: Add rough Q2 visualization check #154 Addresses comment from @ElDeveloper * STY: Remove blank lines in match_inputs() Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]> * STY: more blank line removals in docstring Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]> * STY: rm blank lines in print_if_dropped docstring Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]> * MNT: warn instead of printing re: sample dropping Tests haven't been updated yet -- will do so when --ignore-missing-samples option added in. (So this will currently break the tests.) This represents part of the work on addressing @ElDeveloper's comments on #154. * ENH: add UI skeleton for no-data sample/feat flags Per suggestion from @ElDeveloper in #154 * STY: make _plot inputs prettier * DOC: add ref to emperor --ignore-missing-samples * DOC: Remove 'standalone' instructions in README Just for now. When we resolve #140, we should add these instructions back in (likely we'll also have to adjust these when we get to the 'initial release' of Empress on PyPI / conda-forge / etc.) * DOC: switch feature/sample flag order, imprv docs * ENH: Add @ElDeveloper's suggested filtering flags This entailed substantial restructuring of match_inputs(). I also completely deleted warn_if_dropped(), because it was honestly easier to replace it with custom error messages for each of its 3 usages. (Also, that thing was like 50 lines of docstring / infrastructure for 8 lines of code. It was gnarly. :P) This isn't done yet! I still need to test this new behavior thoroughly, and to update the tests for the old functionality accordingly. * MNT: Avoid redundant table DF transpositions #155 * BUG: don't display useless warning in most cases * TST: reduce tests to just one working one will add more back (with relevant changes to work with new behavior) soon * DOC: add TODO note re empty checking * TST: add back "simple" matching error tests * TST: add + beef up tests of matching warnings, etc * TST: add --p-ignore-missing-samples tests * TST: add another cornercase test * TST: test final "warning" in matching func for now also fixed a bug in prev test i just added in, and removed extraneous comment * TST: Add other check for extra s.m. sample warning I think I'm satisfied with the new matching behavior tests, at least for now * DOC: update example QZV :) * MNT: don't warn on dropped samples from s.metadata See new comment for justification. Addresses comment from @ElDeveloper. Co-authored-by: Yoshiki Vázquez Baeza <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In this test data, there are two samples:
S1andS2, and every feature in the tree is present in one of these samples. When neitherS1norS2are in the metadata, this error occurs; when at least one of the two samples have metadata, the Empress visualization loads (i.e. the loading animation stops and the visualization becomes interactive).this (and related errors re: "matching" between datasets) should be solvable by adding in validation code that explicitly checks that:
Ideally, "problematic" or impossible cases would cause an error when the user tries to create a visualization from the command line, rather than later on in the JS side of things.
I know that Qurro has code to do similar validation here, and there are plenty of other packages from the lab that do similar things (e.g. Songbird here). The main time-consuming task here will just be adding lots of tests to make sure that whatever validation code we add behaves as intended.
The text was updated successfully, but these errors were encountered: