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

sorted all menus #545

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

sorted all menus #545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f54080
sorted all menus
kwcantrell Aug 12, 2021
4a769a7
address @fedarko's comments
kwcantrell Aug 26, 2021
e3a15b5
Apply suggestions from code review
kwcantrell Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions empress/support_files/js/empress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2712,7 +2712,7 @@ define([
};

/**
* Returns a list of sample categories.
* Returns a sorted list of sample categories.
*
* If this.isCommunityPlot is false (no table / sample metadata were
* provided), this just returns [].
Expand All @@ -2721,7 +2721,7 @@ define([
*/
Empress.prototype.getSampleCategories = function () {
if (this.isCommunityPlot) {
return this._biom.getSampleCategories();
return util.naturalSort(this._biom.getSampleCategories());
} else {
return [];
}
Expand Down Expand Up @@ -2849,12 +2849,12 @@ define([
};

/**
* Returns an array of feature metadata column names.
* Returns a sorted list of feature metadata column names.
*
* @return {Array}
*/
Empress.prototype.getFeatureMetadataCategories = function () {
return this._featureMetadataColumns;
return util.naturalSort(this._featureMetadataColumns);
};

/**
Expand Down Expand Up @@ -3792,5 +3792,36 @@ define([
this.redrawBarPlotsToMatchLayout();
};

/**
* Returns the feature metadata value at a f. m. column for a node.
*
* @param {Number} node Postorder position of a node in the tree.
* @param {String} col Name of a feature metadata column.
*
* @return {String} The col variable value for the node or undefined if
* no value exists.
*/
Empress.prototype.getNodeFeatureMetadataValue = function (node, col) {
var colIndx = _.indexOf(this._featureMetadataColumns, col);
if (_.has(this._tipMetadata, node)) {
return this._tipMetadata[node][colIndx];
}
if (_.has(this._intMetadata, node)) {
return this._intMetadata[node][colIndx];
}
return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

};

/**
* Checks to see if a node has feature metadata.
*
* @param {Number} node Postorder position of a node in the tree.
*
* @return true if node has feature metadata, false otherwise.
*/
Empress.prototype.hasFeatureMetadata = function (node) {
return _.has(this._tipMetadata, node) || _.has(this._intMetadata, node);
};

return Empress;
});
20 changes: 7 additions & 13 deletions empress/support_files/js/select-node-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,23 +288,17 @@ define(["underscore", "util"], function (_, util) {
) {
if (this.hasFeatureMetadata) {
this.fmTable.innerHTML = "";
var mdObj;
if (tipOrInt === "tip") {
mdObj = this.empress._tipMetadata;
} else if (tipOrInt === "int") {
mdObj = this.empress._intMetadata;
} else {
throw new Error("Invalid tipOrInt value: " + tipOrInt);
}
if (_.has(mdObj, nodeName)) {
if (this.empress.hasFeatureMetadata(nodeName)) {
var headerRow = this.fmTable.insertRow(-1);
Comment on lines -299 to 292
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

var featureRow = this.fmTable.insertRow(-1);
for (var x = 0; x < this.fmCols.length; x++) {
var colName = this.fmCols[x];
for (var col of this.fmCols) {
var colCell = headerRow.insertCell(-1);
colCell.innerHTML = "<strong>" + colName + "</strong>";
colCell.innerHTML = "<strong>" + col + "</strong>";
var dataCell = featureRow.insertCell(-1);
dataCell.innerHTML = mdObj[nodeName][x];
dataCell.innerHTML = this.empress.getNodeFeatureMetadataValue(
nodeName,
col
);
}
show(this.fmTable);
hide(this.fmNoDataNote);
Expand Down
4 changes: 2 additions & 2 deletions tests/test-barplots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ require([
_.each(empress._barplotPanel.layers, function (layer, i) {
// Basic information about the visualization -- should be the same
// across every layer
equal(layer.fmCols, scope.testData.fmCols);
equal(layer.smCols, empress._barplotPanel.smCols);
deepEqual(layer.fmCols, scope.testData.fmCols);
deepEqual(layer.smCols, empress._barplotPanel.smCols);
equal(layer.barplotPanel, empress._barplotPanel);
equal(layer.layerContainer, empress._barplotPanel.layerContainer);
// Check that the "num" and "unique num" of each barplot layer were
Expand Down