sorted all menus #545
sorted all menus #545
Conversation
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv |
There was a problem hiding this comment.
Thanks @kwcantrell! Some of the abstractions done here from select-node-menu.js are really nice -- moving this sort of logic out of the node menu and into empress is a good idea.
That being said, I am not sure that the actual end goal of this PR, sorting all metadata columns, is always going to be beneficial. For sample metadata columns I can definitely see the benefit, but for feature metadata (esp. when the only feature metadata in use is QIIME 2 taxonomy feature metadata, which I think is a pretty common use case right now) not using the default ordering will make it so that Confidence is always the first listed value (as seen on the moving pictures dataset) --
... in my opinion this will almost always be inconvenient for users, since showing the taxonomy columns before the other feature metadata is usually what we want. Especially w/r/t taxonomy metadata, the confidence information describes the confidence of the taxonomic annotations, so I think it makes sense to list it after taxonomy rather than beforehand.
That being said, I understand that Emperor already sorts its feature metadata columns when listing them in the biplot coloring menu, so ... if we want to make this change anyway I can deal with it :)
| if (_.has(mdObj, nodeName)) { | ||
| if (this.empress.hasFeatureMetadata(nodeName)) { | ||
| var headerRow = this.fmTable.insertRow(-1); |
There was a problem hiding this comment.
It doesn't look like the modified code considers tipOrInt any more. Is this due to nodeName no longer being the node name but actually just being the postorder position of a node in the tree (in which case there is no longer any ambiguity, since each position should be unique relative to all other nodes)? If so, we should update this function / its documentation, and probably other stuff in this module that still assumes nodes are referred to by their names. (That change is not really in scope of this PR, though -- I'm fine with delegating that to another issue, since I imagine doing it right will be a huge pain.)
There was a problem hiding this comment.
Correct, tipOrInt no longer being used. empress.hasFetaureMetadata and empress.getNodeFeatureMetadataValue replace its functionality. I don't believe ambiguity is an issue since tip names must be unique thus if nodeName is in tipMetadata then by definition it cannot be in intMetadata and vice-versa. Right?
Assuming I am not looking over anything, I'll remove tipOrInt from the doc string.
| if (_.has(this._intMetadata, node)) { | ||
| return this._intMetadata[node][colIndx]; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Wait, should this ever happen in practice? At least in the context of makeFeatureMetadataTable() I don't think it should, since we've already called Empress.hasFeatureMetadata(). I vote we throw an error here instead, unless I am missing something.
There was a problem hiding this comment.
We can throw an error. I choose to return undefined in case the calling function wants to do something special in situations where no metadata exists (which I guess can also be handled by a try catch block).
|
The main motivation behind sorting everything came up while demonstrating EMPress to the health team during Tuesdays meeting. They have a lot of metadata columns and the current method made it fairly difficult to find things. I think for more general uses, this will be the case. It maybe nice to list confidence after taxonomy but I think that small convenience is outweighed by the possible frustration of having to search for every other category. I could be wrong though 😄 |
|
Fair enough! Since there's a compelling reason for it, I don't mind this change.
Agreed. We miiiight be able to get the best of both worlds by renaming |
Co-authored-by: Marcus Fedarko <[email protected]>
|
Looks like McHelper is failing again... 😢 |
|
@fedarko I believe I addressed all your comments. |
|
Did you try rerunning the jobs? I just restarted them. It’s likely an IP related problem(?). 🐧
Yoshiki Vázquez-Baeza
… On Aug 26, 2021, at 12:51 PM, kwcantrell ***@***.***> wrote:
Looks like McHelper is failing again... 😢
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Yep, artifacts should be updated now. |
|
@fedarko any chance you can review and merge if things look good to you? |
|
@ElDeveloper yep, I will do my best to get to the remaining open pull requests ASAP. |
|
Thanks so much!
Yoshiki Vázquez-Baeza
… On Oct 26, 2021, at 12:48 PM, Marcus Fedarko ***@***.***> wrote:
@ElDeveloper yep, I will do my best to get to the remaining open pull requests ASAP.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
This sorts all drop down menus and the node select menu. This should hopefully make it much easier to find things 😄