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.

Sign up for GitHub

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

Jump to bottom

Clicking on nodes that aren't present in the feature table causes errors #314

Closed
fedarko opened this issue Aug 8, 2020 · 1 comment · Fixed by #432
Closed

Clicking on nodes that aren't present in the feature table causes errors #314

fedarko opened this issue Aug 8, 2020 · 1 comment · Fixed by #432
Assignees
@fedarko
Labels
bug
Milestone
Second Beta Release

Comments

@fedarko
Copy link
Collaborator

fedarko commented Aug 8, 2020 •
edited
Loading

This occurs when --p-no-filter-unobserved-features-from-phylogeny is passed (i.e. the tree is not shorn to the features present in the table).

Clicking on such a tip causes the following error:

Uncaught Error: Feature ID "tipIDgoeshere" not in BIOM table.
    at BIOMTable._getFeatureIndexFromID (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/biom-table.js:120)
    at BIOMTable.getObsCountsBy (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/biom-table.js:307)
    at Empress.computeTipSamplePresence (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/empress.js:2574)
    at SelectedNodeMenu.showLeafNode (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/select-node-menu.js:246)
    at SelectedNodeMenu.showNodeMenu (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/select-node-menu.js:194)
    at HTMLButtonElement.click (/_/03wu22ds7qnm/b3f410c8-e22d-41d6-b8d7-0f56fc401a3e/data/support_files/js/select-node-menu.js:46)

The selected node menu code assumes in multiple places that the tip is in the table (this.empress.computeTipSamplePresence() and SelectedNodeMenu.makeSampleMetadataTable(), among probably other places), which is a problem. A lot of this is likely my bad.

From what I can tell, computeIntSamplePresence() when clicking on an internal node where no descendant tips are in the table doesn't throw any errors, but a Sample Presence Info table with all 0s is still shown in the resulting node selection menu; this should be hidden.

For reference, I think the expected behavior when clicking on a node not in the table is that the user should still be able to see the node's name and any feature metadata available for it, with the "Sample Presence Information" stuff hidden.
@fedarko fedarko added the bug label Aug 8, 2020
@fedarko
Copy link
Collaborator Author

fedarko commented Oct 13, 2020

@gibsramen / @gwarmstrong this is the issue with un-sheared trees I mentioned just now -- I think the problematic code is around here:

empress/empress/support_files/js/select-node-menu.js

Lines 256 to 264 in a76ebf2

if (this.empress.isCommunityPlot) {
// 2. Add sample presence information for this tip
// TODO: handle case where tip isn't in table, which happens if
// --p-no-shear-tree is passed
// (https://github.com/biocore/empress/issues/314)
var ctData = this.empress.computeTipSamplePresence(
node,
this.fields
);

@kwcantrell kwcantrell added this to the Second Beta Release milestone Oct 20, 2020
fedarko added a commit to fedarko/empress that referenced this issue Oct 21, 2020
@fedarko
Prevent errors+warn on tips not in table biocore#314
508d29b
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
Make int sample presence handle bad case&test biocore#314
7420d21 
still gotta clean up tests, and actually integrate this into the
selection menu, and etc. but a start.
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
tidy up biocore#314 int test
7cb35d1
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
Show warning for int nodes w/ all tips not in tbl
9e8ece9 
Closes biocore#314.

However, the UI here could still be improved a lot! So gonna tidy
things up a bit before getting this PR in. May knock out biocore#272 while
we're at it.
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
TODOs for menu fixing biocore#314
5402ce0
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
Reorganize and restyle selection menu
2c01ba2 
Not done yet (gotta move stuff around for SM), but this already
makes the organization here clearer i think. Having FM and SM be
separate clearly marked sections should make things easier to
understand, since it will be very easy to say "oh this node has no
feature metadata" or "oh this node isn't in the table" (biocore#314).
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
some biocore#314 esque menu fixes
b7fef8c 
have things planned out, just gotta go through and refactor stuff
fedarko added a commit to fedarko/empress that referenced this issue Oct 22, 2020
@fedarko
Refactor SM stuff :D
72a8b4f 
Closes biocore#314 for good, I think. Some bugs (biocore#272 rears its head again)
and i wanna test this, but this is already a big improvement
@fedarko fedarko mentioned this issue Oct 23, 2020
Merged
ElDeveloper added a commit that referenced this issue Nov 16, 2020
@fedarko @ElDeveloper
Fix clicking on nodes not in table; refactor and test node selection …
dc146db 
…menu; fix "Add" button (#432)

* ENH: add BiomTable.hasFeatureID

* Prevent errors+warn on tips not in table #314

* fix warn msg

* tidy styling/stuff

* Make int sample presence handle bad case&test #314

still gotta clean up tests, and actually integrate this into the
selection menu, and etc. but a start.

* tidy up #314 int test

* child -> descendant tips in int sample presence

* TST: beef up int sample presence tests

* STY: prettifififify

* Show warning for int nodes w/ all tips not in tbl

Closes #314.

However, the UI here could still be improved a lot! So gonna tidy
things up a bit before getting this PR in. May knock out #272 while
we're at it.

* polish selected node menu a bit

Still kinda broken, but I think i have an idea of how to fix it

* TST: Add selected node menu test skeleton :D

Because manually testing this is going to give me a stroke

* TST: test a sel node menu error case; chg init'ing

Didn't seem like there was much of a reason to keep initialize out
of the selected node menu constructor, so just went ahead and moved
it there.

* TST/MNT: add + tidy up ssn tests; tidy err cases

since the tip case is afaict literally impossible to test

* More s.node menu tests; revamp HTML ._.

not thrilled about doing this the brute force way but it works

* beef up showNodeMenu tip test

* rename SelectedNodeMenu.notes to .smNotes

for clarity's sake, since .notes seems way more general than
the actual purpose this element serves

* beef up tip menu test

* MNT: simplify showing/hiding HTML eles in S.N.Menu

was getting tired of writing stuff out

* TODOs for menu fixing #314

* STY

* BUG: hide add ui eles after all sm fields added

Closes #272.

* Reorganize and restyle selection menu

Not done yet (gotta move stuff around for SM), but this already
makes the organization here clearer i think. Having FM and SM be
separate clearly marked sections should make things easier to
understand, since it will be very easy to say "oh this node has no
feature metadata" or "oh this node isn't in the table" (#314).

* STY: pret

* MNT: take advantage of makeFMtable being nonstatic

This makes calling this, and doing state-modifying stuff from
within this function, so much less of a pain. not sure why i thought
this would be a good idea back in june lmao

* MNT; take advantage of sm table being nonstatic

* some #314 esque menu fixes

have things planned out, just gotta go through and refactor stuff

* Refactor feature metadata in s.node menu

* css todo

* Refactor SM stuff :D

Closes #314 for good, I think. Some bugs (#272 rears its head again)
and i wanna test this, but this is already a big improvement

* Fix #272 bug (...again.)

ok this seems good. just gotta add tests and then pr should be ready

* STY

* TST: unbreak s. node menu tests

* Simplify s. node menu js tests :D

* TST: add an int node s node menu test

* Apparently qunit is case sensitive wrt "teardown"

:thinking:

I can't believe I just spent like 15 minutes trying to debug this lmao

* TST: test #272 case

wound up being a surprisingly large pain. turns out the (main) reason
was that we actually shouldn't have been creating a new selected
node menu in the tests; we should've just used the test data's
copy. Stuff was being initialized twice, so there were 6 (not 3)
s.m. fields in the <select>, and i was tearing my hair out lmao

* TST: test ambiguous internal node menu case

* MNT/TST: simplify js test code

* MNT/TST: abstract out fm table checking in snm tst

* test sm table population

* tidy up selected node menu code formatting

* STY: appease jshint by declaring a var in scope

WHOOPS

* TST: test sm notes txt in one of the cases

* checktips fix

* Update now-outdated-due-to-#400 comment

Thanks @ElDeveloper for pointing this out!

* Update empress/support_files/js/select-node-menu.js

Co-authored-by: Yoshiki Vázquez Baeza <[email protected]>

* HACK: Fix adjacent sm/fm section border CSS thing

YEEESH

* ENH: Actually, nvm, ditch menu section borders

But make things look a lot nicer -- e.g. show note in sm section when
no columns selected, remove extraneous <br />, etc.

* cats can have a little <hr />, as a treat

* TST: unbreak select node menu tests ._.

* STY: prettify test

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
Assignees

@fedarko fedarko

Labels
bug
Projects
None yet
Milestone
Second Beta Release
2 participants
@fedarko @kwcantrell