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

React finance precursor #2525

Merged
merged 10 commits into from Apr 5, 2018

Conversation

Projects
None yet
3 participants
@alexcjohnson
Copy link
Contributor

commented Apr 4, 2018

Various fixes to our supplyDefaults framework to make Plotly.react happier. Since this is no longer part of fixing finance charts with react per se (#2510 is going a different route now), making a separate PR for these here.

cc @etpinard

delete trace.zmin;
delete trace.zmax;
}
});

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 4, 2018

Author Contributor

Having collected all the patches we currently need to get fullJson to match up from run to run, we can then go about cleaning them up in subsequent PRs. In the meantime @nicolaskruchten @VeraZab the editor may need to be aware that these attributes are inconsistently available in the meantime 🙈

Note that I used Plotly.Plots.graphJson to generate fullJson, which already removes underscored attributes and functions.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

Brilliant test

This comment has been minimized.

Copy link
@VeraZab

VeraZab Apr 5, 2018

Collaborator

@alexcjohnson only zmin and zmax?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

@VeraZab zmin and zmax are actually not a problem for you, I don't think, as they only apply to contourcarpet where they're not even attributes (though they really should be... and if we make them into bon-a-fide attributes this error will go away).

It's the ones farther up that might cause headaches: tick0, dtick, and (for 3D and maybe occasionally for 2D) range. I think this is only when some other attribute indicates these attributes are auto-determined, so it still may not be an issue as you probably don't even want to display them in these cases, but I'm not sure about that.

The way to test would be to arrange a situation where you DO see those attributes, then make some totally unrelated minor change (tweak a color or something in a different panel) and go back and see if they're still where they're supposed to be.

Or just ignore it for now 😎 but if you happen to come across a bug that looks like ^^ yell at me to fix this.

// 'regular' loop - make sure container traces (eg carpet) calc before
// contained traces (eg contourcarpet)
for(i = 0; i < fullData.length; i++) calci(i, true);
for(i = 0; i < fullData.length; i++) calci(i, false);

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 4, 2018

Author Contributor

Because I moved convertColumnData and setConvert for carpet traces from the supplyDefaults step to the calc step, we need to ensure carpet.calc happens before contourcarpet.calc and scattercarpet.calc. I'm not sure if we're ever going to have another situation like this where one trace essentially lives inside another, but in case we do this seemed like a reasonably generic way to handle it.

trace = fullData[i];
_module = trace._module;

if(!!_module.isContainer !== isContainer) return;

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

Ha. Nice fix. I wonder if other carpet-dependent blocks (e.g. here) could be simplified (or at least generatlized) using _module.isContainer?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

Probably some such blocks could... though I don't see much to do with that carpetDependents loop. It might be nice to move it out of plots/plots and into the carpet module, but probably not in this PR.

delete trace.zmin;
delete trace.zmax;
}
});

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

Brilliant test

if(len < high.length) traceOut.high = high.slice(0, len);
if(len < low.length) traceOut.low = low.slice(0, len);
if(len < close.length) traceOut.close = close.slice(0, len);
traceOut._inputLength = len;

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

Any reason for naming this _inputLength? Parcoords and splom use _commonLength to stash their common dimensions[i].values length - which I guess is a little different than the finance trace case here and the regular traceOut._length used in (all?) other traces.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

This will be converted to _length when I de-transform these traces, but I didn't want to use that (or _commonLength) here since the post-transform traces have different lengths.

t.xp = trace.xp = map2dArray(trace.xp, x, xa.c2p);
t.yp = trace.yp = map2dArray(trace.yp, y, ya.c2p);
t.xp = trace._xp = map2dArray(trace._xp, x, xa.c2p);
t.yp = trace._yp = map2dArray(trace._yp, y, ya.c2p);

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

Are these xp and yp values used anywhere? Here they appear to be reset during the plot step.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

ha good call. Nothing fails if you 🔪 these lines, which is good because the calc step had better not generate pixel positions, the axis scaling isn't known yet at that point!
Removed in 0221510

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

wow, then map_2d_array became totally unused -> b0af416

@@ -380,7 +380,7 @@ plots.supplyDefaults = function(gd) {
if(_module.cleanData) _module.cleanData(newFullData);
}

if(oldFullData.length === newData.length) {
if(oldFullData.length === newFullData.length) {

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

How hard would it be to 🔒 this fix?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2018

Author Contributor

I was going to add a Plotly.react test in transform_groupby_test for this and diffing on _fullInput, hence c2b11dc, since that's the obvious place where fullData will have a different length from data after finance is converted. But on second thought perhaps that can of worms should wait for #2508? I'll make a note in that issue if you 👍. The fact that all the existing Plotly.react and other tests pass means these didn't cause problems at least.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

But on second thought perhaps that can of worms should wait for #2508?

sounds good 👌

for(i = 0; i < oldFullData.length; i++) {
trace = newFullData[i];
trace = newFullData[i]._fullInput;
if(seenUIDs[trace.uid]) continue;

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 5, 2018

Member

similarly here, can we 🔒 this thing?

@@ -65,10 +65,6 @@ module.exports = function calc(gd, trace) {
// create conversion functions that depend on the data
trace.setScale();

// Convert cartesian-space x/y coordinates to screen space pixel coordinates:
t.xp = trace._xp = map2dArray(trace._xp, x, xa.c2p);
t.yp = trace._yp = map2dArray(trace._yp, y, ya.c2p);

This comment has been minimized.

Copy link
@etpinard

alexcjohnson added some commits Apr 5, 2018

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Great PR 💃

@alexcjohnson alexcjohnson merged commit acc7d81 into master Apr 5, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@alexcjohnson alexcjohnson deleted the react-finance-precursor branch Apr 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.