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

Fix/histograms don't match the scatter plot #80

Merged
merged 6 commits into from
Oct 29, 2021
Merged

Fix/histograms don't match the scatter plot #80

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
733e436
Fix type errors
schoinh Oct 27, 2021
46544c3
Fix bug
schoinh Oct 27, 2021
6f39210
Fix formatting
schoinh Oct 27, 2021
6f7b892
Make util function syncNullValues
schoinh Oct 27, 2021
4399f59
Use copies of data for handling nulls
schoinh Oct 28, 2021
0552877
Remove unused import
schoinh Oct 28, 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
4 changes: 2 additions & 2 deletions src/containers/MainPlotContainer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ interface PropsFromState {
xDropDownOptions: MeasuredFeatureDef[];
xTickConversion: TickConversion;
yTickConversion: TickConversion;
xValues: number[];
yValues: number[];
xValues: (number | null)[];
yValues: (number | null)[];
schoinh marked this conversation as resolved.
Show resolved Hide resolved
proteinNames: string[];
}

Expand Down
5 changes: 5 additions & 0 deletions src/containers/MainPlotContainer/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
} from "../../state/selection/selectors";
import { TickConversion } from "../../state/selection/types";
import { Annotation, ContinuousPlotData, GroupedPlotData } from "../../state/types";
import { syncNullValues } from "../../util";

function isGrouped(plotData: GroupedPlotData | ContinuousPlotData): plotData is GroupedPlotData {
return plotData.groupBy === true;
Expand All @@ -64,6 +65,10 @@ export const getMainPlotData = createSelector(
colorsForPlot,
categoricalFeatures
): GroupedPlotData | ContinuousPlotData => {
// Only preserve values at indices where both x and y values are not null,
// because a coordinate like (3, null) won't be plotted anyway and produces
// inaccurate histograms.
syncNullValues(xValues, yValues);
// for datasets that have a lot of null values,
// if the whole array is null it throws an error
if (!filter(xValues).length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is something fishy to me about this filter call, too. is this just supposed to be a check for whether the array contains all null values? (what if it contains all 0 values?) filter seems to create a whole new array, which means we're doing a very unnecessary data copy here. I feel like this and the syncNullValues could be combined into something more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point about all 0 values (and the data copy). I should be able combine the two operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize it is outside the scope of the PR but I made the mistake of trying to actually understand the code. Hopefully it is not too involved to clean it up. :(

Expand Down
8 changes: 4 additions & 4 deletions src/state/selection/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export const getMeasuredValues = createSelector([getPerCellDataForPlot], (plotFo

export const getXValues = createSelector(
[getMeasuredValues, getPlotByOnX],
(measuredData: MappingOfMeasuredValuesArrays, plotByOnX: string): number[] => {
(measuredData: MappingOfMeasuredValuesArrays, plotByOnX: string): (number | null)[] => {
if (measuredData[plotByOnX]) {
return measuredData[plotByOnX];
}
Expand All @@ -147,7 +147,7 @@ export const getXValues = createSelector(

export const getYValues = createSelector(
[getMeasuredValues, getPlotByOnY],
(measuredData: MappingOfMeasuredValuesArrays, plotByOnY: string): number[] => {
(measuredData: MappingOfMeasuredValuesArrays, plotByOnY: string): (number | null)[] => {
if (measuredData[plotByOnY]) {
return measuredData[plotByOnY];
}
Expand All @@ -157,7 +157,7 @@ export const getYValues = createSelector(

export const getFilteredXValues = createSelector(
[getFilteredMeasuredValues, getPlotByOnX],
(measuredData: MappingOfMeasuredValuesArrays, plotByOnX: string): number[] => {
(measuredData: MappingOfMeasuredValuesArrays, plotByOnX: string): (number | null)[] => {
if (measuredData[plotByOnX]) {
return measuredData[plotByOnX];
}
Expand All @@ -167,7 +167,7 @@ export const getFilteredXValues = createSelector(

export const getFilteredYValues = createSelector(
[getFilteredMeasuredValues, getPlotByOnY],
(measuredData: MappingOfMeasuredValuesArrays, plotByOnY: string): number[] =>
(measuredData: MappingOfMeasuredValuesArrays, plotByOnY: string): (number | null)[] =>
measuredData[plotByOnY] || []
);

Expand Down
56 changes: 24 additions & 32 deletions src/state/selection/test/selectors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,62 +13,58 @@ import {
} from "../selectors";

describe("Selection selectors", () => {


const newMockState = mockState;

describe("getFilteredXValues selector", () => {
it("returns an array of values that correspond to the currently selected x value", () => {
const state: State = {
...newMockState,
selection: {
...newMockState.selection,
plotByOnX: "apical-proximity",
},
};
const result: (number | null)[] = getFilteredXValues(state);
const newState = {
...state,
selection: {
...newMockState.selection,
plotByOnX: "cell-segmentation",
},
};
const feature1Values = [-0.25868651080317, -0.1];
const feature2Values = [1, 0];

const state: State = {
...newMockState,
selection: {
...newMockState.selection,
plotByOnX: "apical-proximity",
},
};
const result: number[] = getFilteredXValues(state);
const newState = {
...state,
selection: {
...newMockState.selection,
plotByOnX: "cell-segmentation",
},
};
const feature1Values = [-0.25868651080317, -0.1];
const feature2Values = [1, 0];

const newResult: number[] = getFilteredXValues(newState);
expect(result).to.deep.equal(feature1Values);
expect(newResult).to.deep.equal(feature2Values);
expect(result.length).to.equal(newResult.length);
const newResult: (number | null)[] = getFilteredXValues(newState);
expect(result).to.deep.equal(feature1Values);
expect(newResult).to.deep.equal(feature2Values);
expect(result.length).to.equal(newResult.length);
Comment on lines +20 to +41
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just all whitespace change

});
});
describe("getFilteredYValues selector", () => {
it("returns an array of values that correspond to the currently selected y value", () => {

const state: State = {
...newMockState,
selection: {
...newMockState.selection,
plotByOnY: "apical-proximity",
},
};
const result: number[] = getFilteredYValues(state);
const result: (number | null)[] = getFilteredYValues(state);
const newState = {
...state,
selection: {
...newMockState.selection,
plotByOnY: "cell-segmentation",
},
};
const newResult: number[] = getFilteredYValues(newState);
const newResult: (number | null)[] = getFilteredYValues(newState);
expect(result).to.not.deep.equal(newResult);
expect(result.length).to.equal(newResult.length);
});
});
describe("getSelectedGroupKeys selector", () => {
it("it returns the keys of the selected groups, may be strings or numbers", () => {

const state: State = {
...newMockState,
selection: {
Expand All @@ -84,7 +80,6 @@ describe("Selection selectors", () => {
expect(result).to.deep.equal(["id1", "id2"]);
});
it("it returns an empty array if no selected groups", () => {

const state: State = {
...newMockState,
};
Expand All @@ -95,7 +90,6 @@ describe("Selection selectors", () => {
});
describe("getSelectedSetTotals selector", () => {
it("it returns an array where each value is the total number of points in that group", () => {

const state: State = {
...newMockState,
selection: {
Expand All @@ -112,7 +106,5 @@ describe("Selection selectors", () => {
const result: number[] = getSelectedSetTotals(state);
expect(result).to.deep.equal([total1, total2]);
});

});

});
8 changes: 4 additions & 4 deletions src/state/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ export interface SelectedGroupDatum {
export interface ContinuousPlotData {
color: Color | Color[] | number | number[];
ids?: string[];
x: number[];
y: number[];
x: (number | null)[];
y: (number | null)[];
customdata?: string[];
opacity?: number[];
groupBy?: boolean;
Expand All @@ -83,8 +83,8 @@ interface GroupSettings {
}
export interface GroupedPlotData {
ids?: string[];
x: number[];
y: number[];
x: (number | null)[];
y: (number | null)[];
customdata?: string[];
groupBy: boolean;
groups: number[] | string[];
Expand Down
16 changes: 16 additions & 0 deletions src/util/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,19 @@ export function isDevOrStagingSite(host: string): boolean {
// first condition is for testing with no client
return !host || host.includes("localhost") || host.includes("staging") || host.includes("stg");
}

export function syncNullValues(array1: (number | null)[], array2: (number | null)[]): void {
/* At every index where one array has a null value, the other array must also have a null value */

if (array1.length !== array2.length) {
console.error("Cannot syncNullValues between two arrays because they have unequal length")
return;
}
for (let i = 0; i < array1.length; i++) {
if (array1[i] === null) {
array2[i] = null;
} else if (array2[i] === null) {
array1[i] = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it should be happening in the selector that gets the values for the histogram? What's the reason for doing it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is it in a util function? so it's easier to test 😅 Or were you asking another question?

Copy link
Contributor Author

@schoinh schoinh Oct 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The values that go in the histograms are just mainPlotData.x and mainPlotData.y,

cell-feature-explorer/src/containers/MainPlotContainer/selectors.ts

Line 258 in f313c72

makeHistogramPlotX(mainPlotData.x),
so I figured I would just do this null-syncing operation in getMainPlotData selector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry, I didn't read this carefully enough and thought the util function was being called from a component, not another selector. What I meant was I was thinking we'd have a "getHistogramData" selector that fed into the main plot data, but this works. I think you could keep the function in the selector file since it's only being called from there, and it doesn't seem like a general function other modules are going to use.

Generally there is the convention that anytime you're reading from state, like in a selector, you make a copy instead of directly manipulating the data, and you only manipulate the data through actions. I can't think of any issue this code as is would cause (you could even make the case we should just be doing this before we save the data in state) but it does look odd to me

Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern here is that you are actually modifying the array. Is that ok? Are you actually changing data that shouldn't be changed? Like if you have (x=3,y=null) and you change the 3 to a null but then want to plot a different Y feature, is the x=3 gone forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'll rework this to make copies of the data instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should say that I am not aware of whether you are already starting with copies when you actually arrive in this function, or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not starting with copies. But I should. Either that or produce copies in here

}
}
}
12 changes: 11 additions & 1 deletion src/util/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { expect } from "chai";

import { bindAll, isDevOrStagingSite } from "../";
import { bindAll, isDevOrStagingSite, syncNullValues } from "../";

describe("General utilities", () => {
describe("bindAll", () => {
Expand Down Expand Up @@ -71,4 +71,14 @@ describe("General utilities", () => {
expect(result).to.deep.equal(Array(productionSite.length).fill(false));
});
});
describe("syncNullValues", () => {
it("syncs null values between two arrays", () => {
const array1 = [null, 3, 5, null];
const array2 = [2, 4, null, null];
syncNullValues(array1, array2);

expect(array1).to.deep.equal([null, 3, null, null]);
expect(array2).to.deep.equal([null, 4, null, null]);
});
});
});