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.

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

Feature/sort datasets #74

Merged
merged 8 commits into from
Oct 11, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion src/containers/App/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function mapStateToProps(state: State) {
isLoading: metadataStateBranch.selectors.getIsLoading(state),
loadingText: metadataStateBranch.selectors.getLoadingText(state),
selectedDataset: selectionStateBranch.selectors.getSelectedDataset(state),
datasets: metadataStateBranch.selectors.getDatasets(state),
datasets: metadataStateBranch.selectors.getDatasetsByNewest(state),
};
}

Expand Down
26 changes: 26 additions & 0 deletions src/state/metadata/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
MITOTIC_STAGE_KEY,
PROTEIN_NAME_KEY,
} from "../../constants";
import { DatasetMetaData } from "../../constants/datasets";
import { State } from "../types";

import {
Expand All @@ -28,6 +29,31 @@ export const getMeasuredFeaturesDefs = (state: State) => state.metadata.measured
export const getFileInfo = (state: State) => state.metadata.cellFileInfo;
export const getClusterData = (state: State) => state.metadata.clusterData;

export const getDatasetsByNewest = createSelector([getDatasets], (datasets) => {
return datasets.sort((a: DatasetMetaData, b: DatasetMetaData) => {
// TODO: We need to sort and group datasets by "megasets"
// and then order those megasets by "newness"
// plus we have plans to have different versions of the same
// dataset contained within one card, so this is a temporary
// sort function to get the newest datasets on top
if (a.name === b.name) {
// if it's the same dataset, return the newest version first
if (a.version > b.version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because version numbers are strings and can be SemVer or CalVer or whateVer, we are probably going to need some better way of doing this comparison. For example, does 2021.10 vs 2021.1 break this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it would, i can fix that

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any great ideas for this except for keeping an internal autoincremented single integer... or having the data creators specify what ordering they want. We rely on the data creators to consistently enforce their own preferred versioning scheme... I guess you could split on "." and hope for the best.

Copy link
Contributor Author

@meganrm meganrm Oct 6, 2021

Choose a reason for hiding this comment

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

yeah, I don't think this PR will cover all cases, it's even only ordering the correct dataset group first because it's just reversed alphabetized. Firebase does have time stamps too, so we could keep them ordered by last updated, however, that only works once we have more concrete ways of grouping mega sets together and grouping versions of the same name into one card

return -1;
} else {
return 1;
}
// otherwise sort by name
} else if (a.name > b.name){
return -1;
} else if (a.name < b.name) {
return 1;
} else {
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

The sorting function has always been kinda confusing for me, but is it possible to just write return a.version - b.version instead of the if-else statements returning -1, 1, and 0, and just write return a.name - b.name under // otherwise sort by name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be return b.version - a.version for newest first? Anyway you see what I'm trying to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, so i've updated it based on Dan's comment, but you're right, I am now comparing numbers so I could just subtract them instead of doing a comparison, that will reduce the number of checks I have to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I miss read this comment a bit. For string comparisons you can't subtract them, this comparison is a pseudo alphabetical sort, mostly I just want the same names grouped together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the sort function, let me know what you think

}
});
})

export const getMeasuredFeatureArrays = createSelector([getPerCellDataForPlot], (dataForPlot: DataForPlot) => {
return dataForPlot.values;
});
Expand Down
79 changes: 74 additions & 5 deletions src/state/metadata/test/selectors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@ import { expect } from "chai";

import { mockState } from "../../test/mocks";
import { State } from "../../types";
import { getMeasuredFeaturesKeys, getProteinNames } from "../selectors";
import { getDatasetsByNewest, getMeasuredFeaturesKeys, getProteinNames } from "../selectors";

describe("Metadata branch selectors", () => {

describe("getMeasuredFeaturesKeys", () => {
it("returns the keys of measured features data", () => {
const state: State = {
...mockState,
};
const result: string[] = getMeasuredFeaturesKeys(state);
expect(result).to.deep.equal(["apical-proximity", "cell-segmentation", "cellular-surface-area", "missing-data"]);
expect(result).to.deep.equal([
"apical-proximity",
"cell-segmentation",
"cellular-surface-area",
"missing-data",
]);
});
});

describe("getProteinNames", () => {
it("returns names of the proteins in the dataset", () => {
const state: State = {
Expand All @@ -24,8 +27,74 @@ describe("Metadata branch selectors", () => {
const result: string[] = getProteinNames(state).sort((a: string, b: string) => {
return b > a ? 1 : -1;
});
expect(result).to.deep.equal(["Nucleophosmin", "Delta-actin", "Beta-catenin", "Beta-actin", "Alpha-actinin-1"]);
expect(result).to.deep.equal([
"Nucleophosmin",
"Delta-actin",
"Beta-catenin",
"Beta-actin",
"Alpha-actinin-1",
]);
});
});

describe("getDatasetsByNewest", () => {
it("returns the dataset card data in the order of newest to oldest", () => {
const dataset1 = {
name: "name1",
version: "2020.1",
};
const dataset2 = {
name: "name1",
version: "2021.1",
};
const state: State = {
...mockState,
metadata: {
datasets: [dataset1, dataset2],
},
};
const result: string[] = getDatasetsByNewest(state);
expect(result).to.deep.equal([dataset2, dataset1]);
});
it("returns the dataset card data in the order of newest to oldest", () => {
const dataset1 = {
name: "name1",
version: "2021.1",
};
const dataset2 = {
name: "name1",
version: "2021.2",
};
const state: State = {
...mockState,
metadata: {
datasets: [dataset1, dataset2],
},
};
const result: string[] = getDatasetsByNewest(state);
expect(result).to.deep.equal([dataset2, dataset1]);
});
it("returns the dataset card data with datasets grouped by name", () => {
const newerName1 = {
name: "name1",
version: "2021.2",
};
const dataset2 = {
name: "name2",
version: "2021.2",
};
const olderName1 = {
name: "name1",
version: "2021.1",
};
const state: State = {
...mockState,
metadata: {
datasets: [newerName1, dataset2, olderName1],
},
};
const result: string[] = getDatasetsByNewest(state);
expect(result).to.deep.equal([dataset2, newerName1, olderName1]);
});
});
});