Make line plot 0/0 proportions have hollow circle #1686
Conversation
There was a problem hiding this comment.
I'm not sure it's worth trying to get rid of the tiny "london underground" horizontal lines at this point.
Approve of everything so far. But merging this would not fix everything in the rather large ticket, so I'm not approving. (I see it's still in Draft though)
| otherArrayKeys.forEach( | ||
| // e.g. x, binLabel etc | ||
| (key) => { | ||
| // initialize empty array if needed | ||
| if (accum[key] == null) accum[key] = []; | ||
| if (get(accum, key) == null) set(accum, key, []); |
There was a problem hiding this comment.
Interested to know why the change here. Style or substance, or both?
Ah - I get it now. It's path-based access, e.g. x['a']['b'] is get(x, 'a.b') - needed for marker.symbol
| el.binSampleSize[0].hasOwnProperty('numeratorN') | ||
| ? el.binSampleSize.map((obj: any) => | ||
| obj.numeratorN || obj.denominatorN ? 'circle' : 'circle-open' | ||
| ) |
There was a problem hiding this comment.
Interestingly, the markers still look like they are connected with lines when they're close together.
Even if we set the data[].mode: 'markers' instead of lines+markers they look like little London Underground logos
The line is coming from the error bars, even though the values are null for the offending 0/0 points. Heres data[].mode: 'none'
It will be a pain to create a separate series with no error bars for the special zero points.
Here's what it looks like when switching off error bars via the UI (and putting lines+markers back on)
| @@ -2126,8 +2138,12 @@ function processInputData( | |||
| binSpec?: BinSpec, | |||
| binWidthSlider?: BinWidthSlider, | |||
| overlayVariable?: Variable, | |||
| colorPaletteOverride?: string[] | |||
| colorPaletteOverride?: string[], | |||
| dependentIsProportion?: boolean | |||
There was a problem hiding this comment.
An alternative to passing yet another argument might be to create at io-ts type for the numeratorN/denominatorN structure, so:
binSampleSize: union([
array(
type({
N: number,
})
),
array(
type({
numeratorN: number,
denominatorN: number,
})
),
]),
would become
export type ProportionSampleSizes = TypeOf<typeof ProportionSampleSizes>;
const ProportionSampleSizes = type({
numeratorN: number,
denominatorN: number,
});
...
binSampleSize: union([
array(
type({
N: number,
})
),
array(ProportionSampleSizes),
]),
Then in the markerSymbol function you could replace the dependentIsProportion with something like
el.binSampleSize.every((bss) => ProportionSampleSizes.is(bss))
I don't know if that makes it any more (or less) readable... (and it probably doesn't fix all the TS issues)
It would be nice to avoid the any and the hasOwnProperty check on just the first array element. But I may just be introducing more TS spaghetti to compensate...
There was a problem hiding this comment.
I might wrap back around to the numeratorN stuff, though there are more pressing TS issues, like the many anys, as you mention.
There was a problem hiding this comment.
I think we might need to explicitly "tag" the LineplotResponse series produced by processZeroOverZeroData with an "isZeroSeries" property?
Introspecting to figure out which symbol to use is tricky. I'm noticing a el.binSampleSize[0] is undefined error when the zeroSeries is empty, though that may be avoidable with a length check - I haven't tried that.
Here's how to reproduce:
LLINEUP -> x=observation date, y=Plasmodium by RDT, num=Yes, denom=all
(basically there are no 0/0s)
|
Had a quick look at the "splitting" code. Was wondering why the nullZeroHack function is still used. But as I say, it was a very quick look. Are you ready for a more detailed look at the new code? |
I plan on removing my added code in A detailed look might be helpful if you have any ideas on how to fix the nasty TS errors that I've been sidestepping with these Also, there's the one property |
The |
There was a problem hiding this comment.
Also, there's the one property facetVariableDetails that's giving me a headache because it's a constant-size tuple (a TS concept, whereas it's just another array in JS), meaning it shouldn't be iterated over as an array, but TS/JS doesn't seem to give any method for weeding out tuples from general arrays. So I'm just excluding it manually, which is annoying. Any thoughts?
That's annoying.
The only thing I can think of is to, explicitly exclude tuples of size 1 and 2 something like this:
type Foo = {
x : number[],
a : [ number, number ] | [ number ]
};
type PickByType<T, Value> = {
[P in keyof T as T[P] extends Value | undefined ? P : never]: T[P];
};
type OmitByType<T, Value> = {
[P in keyof T as T[P] extends Value | undefined ? never : P]: T[P];
};
type Goo = OmitByType<PickByType<Foo, any[]>, [ any ] | [ any, any ]>>;
// Goo is { x: number[] }
It avoids having to name facetVariableDetails directly, but if we decide to allow 3 facet variables (we won't) it will need updating.
In the rest of the code, I don't see any major and potentially impactful uses of any, in terms of technical debt/maintenance. Let me know if I can look again at this though.
| > = []; | ||
| const newZeroSeries: Array<LineplotResponse['lineplot']['data'][number]> = []; | ||
|
|
||
| const stopIndex = hasMissingData |
There was a problem hiding this comment.
I don't think we need to worry about the early stopping before the final element in the lineplot response array that may correspond to the "no data for overlay/facet".
In processInputData, the zeroSplitLineplotData that this function returns is passed through the (somewhat opaque) .some(do_stuff; return breakAfterThisSeries(index)) which will ignore the final element if the user hasn't asked to show the "missing data".
Fixes #1082