Skip to content

Fixing genes' orders in network sheet for ppi and grn#1196

Merged
dondi merged 3 commits intobetafrom
maika-1191
Sep 3, 2025
Merged

Fixing genes' orders in network sheet for ppi and grn#1196
dondi merged 3 commits intobetafrom
maika-1191

Conversation

@ntran18
Copy link
Collaborator

@ntran18 ntran18 commented Apr 25, 2025

No description provided.

@ntran18 ntran18 requested a review from dondi April 25, 2025 21:24
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a quick readability tweak

$(CREATE_NETWORK_MODAL).modal("show");
};

const sortGenes = function (genes, idToSort) {
Copy link
Owner

Choose a reason for hiding this comment

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

Solely for code readability, let’s define constants which assign a name to what the 0 and 1 IDs mean; something like:

Suggested change
const sortGenes = function (genes, idToSort) {
const GRN_STANDARD_NAME_ID = 1;
const PPI_STANDARD_NAME_ID = 0;
const sortGenes = function (genes, idToSort) {

${genesAmount} genes. Please remove some genes from your proposed network.`);
} else {
if (grnState.customWorkbook.type === NETWORK_GRN_MODE) {
grnState.customWorkbook.genes = sortGenes(grnState.customWorkbook.genes, 1);
Copy link
Owner

Choose a reason for hiding this comment

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

That way, over here, we have named constants rather than hardcoded numbers:

Suggested change
grnState.customWorkbook.genes = sortGenes(grnState.customWorkbook.genes, 1);
grnState.customWorkbook.genes = sortGenes(grnState.customWorkbook.genes, GRN_STANDARD_NAME_ID);

@ntran18 ntran18 assigned ntran18 and dondi and unassigned ntran18 Sep 2, 2025
@coveralls
Copy link

Coverage Status

coverage: 80.379%. remained the same
when pulling e558f11 on maika-1191
into a7518fc on beta.

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

LGTM—thanks for making the revisions!

@dondi dondi merged commit 84d3a87 into beta Sep 3, 2025
5 checks passed
@dondi dondi deleted the maika-1191 branch September 3, 2025 05:50
if (grnState.customWorkbook.type === NETWORK_GRN_MODE) {
grnState.customWorkbook.genes = sortGenes(grnState.customWorkbook.genes, GRN_STANDARD_NAME_ID);
const genes = Object.keys(grnState.customWorkbook.genes);
const displayGenes = Object.keys(grnState.customWorkbook.genes).map(g => grnState.customWorkbook.genes[g]);
Copy link
Owner

Choose a reason for hiding this comment

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

@ntran18 can we try something? I have been re-reviewing this PR to see why sorting behavior does not seem to be consistent with what you’re seeing locally, and I have now spotted something that I did not see before

In this version of the code, the sort operation is being done in line 213 above, and this is then converted into an object via Object.fromEntries. Then, Object.keys is used to produce the list which is ultimately sent to the workbook

Here is the wrinkle: Object attributes (the keys here) are not guaranteed to preserve their order. So, even if the object is built from sorted entries, later accesses of the object‘s keys are not guaranteed to retain that order

…which leads me back to the thing I’d like to try: can we move the sorting operation to the very end? I think that means doing the sort on these lines, once the final arrays are created

i.e., do the sort on genes and displayGenes after their contents are finalized

Let’s try that and see if it changes the results. Let me know if you need further elaboration. Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

P.S. Note the specific phrasing here: order preservation of Object.keys is not “guaranteed.” That means the order might be preserved but it is not guaranteed to be that way. This would explain why you can see it work in your local copy, but the behavior may be different for others

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that Object.keys doesn't preserve order. I think it's best if we can switch Object.keys to an array. I will create a new PR for this proposed change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants