Work-in-progress for new cartesian attribute: gridsync

[VictorBezak]

@archmoj @alexcjohnson I attempted to push a new branch to your repo and open a PR there instead of to Master, but I was denied access. Let me know if I need to do this another way, thanks!

---

This is an attempt to adapt the logic originating from https://github.com/VictorBezak/Plotly_Multi-Axes_Gridlines
in order to integrate it with the plotly schema.

This is not a complete implementation, but I am hoping that I might be able to get some feedback as to how I need to further modify and where I need to require gridsync.js in order to get it to be a recognized cartesian layout attribute which I can pass values for testing. This is my first ever open-source contribution and I have less than a year of professional experience, so I apologize for the lack of independence in this first attempt!

Data needed

For this gridsync feature to work properly it needs two pieces of information from the figure:

1. The y-axis data values (current implementation takes them as individual args for only 2 different y-axes, but this should be adapted to accept an arbitrarily sized array of y-axes datasets)
2. The number of gridlines the user wishes to sub-divide their cartesian grid (should be accepted through another cartesian layout attribute)

Attributes to be set

Once these values are received, the gridsync function then needs to override the figure's "range" and the figure's "dtick" values for each y-axis. Once this is done, the gridlines for the y-axes should be in-sync.

Question

Can you offer any insight as to how I can get "gridsync" to become a recognized cartesian layout attribute?

[archmoj]

@archmoj @alexcjohnson I attempted to push a new branch to your repo and open a PR there instead of to Master, but I was denied access. Let me know if I need to do this another way, thanks!

@VictorBezak This is correct. And thanks very much for the PR.

[archmoj]

Question

Can you offer any insight as to how I can get "gridsync" to become a recognized cartesian layout attribute?

Good start!
I noticed you have already added gridSync to the attributes.
As a small note, that should not use any capital letters.
So let's turn that to gridsync.
Then by adding

coerce('gridsync');

to

plotly.js/src/plots/cartesian/axis_defaults.js

Lines 47 to 70 in 2504f1e

	module.exports = function handleAxisDefaults(containerIn, containerOut, coerce, options, layoutOut) {
	var letter = options.letter;
	var font = options.font || {};
	var splomStash = options.splomStash || {};
	var visible = coerce('visible', !options.visibleDflt);
	var axTemplate = containerOut._template || {};
	var axType = containerOut.type || axTemplate.type || '-';
	if(axType === 'date') {
	var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleDefaults');
	handleCalendarDefaults(containerIn, containerOut, 'calendar', options.calendar);
	}
	setConvert(containerOut, layoutOut);
	var autorangeDflt = !containerOut.isValidRange(containerIn.range);
	if(autorangeDflt && options.reverseDflt) autorangeDflt = 'reversed';
	var autoRange = coerce('autorange', autorangeDflt);
	if(autoRange && (axType === 'linear' || axType === '-')) coerce('rangemode');
	coerce('range');
	containerOut.cleanRange();

you would be able to get the value.

@nicolaskruchten @alexcjohnson what do you think of adding this potentially new attribute to the API?

[VictorBezak]

I noticed you have already added gridSync to the attributes.
As a small note, that should not use any capital letters.
So let's turn that to gridsync.

@archmoj Updated

[VictorBezak]

Then by adding

coerce('gridsync');

to
plotly.js/src/plots/cartesian/axis_defaults.js

@archmoj This is good direction, I can make more progress off of this. However, I'll hold off on making any more changes until there's been time for others to give input. If you'd like me to go ahead and progress this implementation further, just let me know

[VictorBezak]

@archmoj This has been stale for quite a while. I'd like to revive this effort and get all Github tests passing against a new commit with the latest Plotly code.

Do you think we could get more eyes on this if I get the PR updated and in a passing state?

[alexcjohnson]

Hi @VictorBezak - thanks for the PR and apologies for not commenting on it the first time around!

Linking in the issue where we all discussed some of this: #1962

Feels to me like the cleanest way to expose this is with a new tickmode value - I had earlier proposed 'match overlay', but the point is tickmode already answers the question "what algorithm do we use to decide where to put ticks" and this setting is really just a different answer to that question rather than answering a new question.

As for the implementation, a couple of things I notice right away:

• The code describes y axes, but this feature should work just as well for overlaid x axes.
• I see some places you attach extra info to the existing axis objects (range_min, range_max) - it's OK to add things, but if they aren't real attributes please prefix them with an underscore (_range_min etc).
• There's some logic re: negative values... I haven't looked into why this is yet, I would have thought that by the time we're calculating the tick (and grid) positions we'd already know the ranges, so it's just a matter of using tick0 and dtick from the main axis to pick tick0 and dtick for the overlay? Anyway if there does need to be special negative logic, let's make sure things still work if both ends of the range are negative, as well as if one or both of the relevant axes is reversed (range[0] > range[1]`) in all three cases.
• I bet we're also going to need some special handling during pan/zoom, if you only change one of these axes and not the other one. So if the left y axis is the main and the right is overlaid, if you grab just the left axis and drag it to pan, the right axis range doesn't change but its ticks and gridlines move and therefore change values, this should be reflected in realtime, not just after you mouse up at the end of the drag. Similarly if you drag the overlaid axis, its ticks shouldn't move but they should change value. We'll want tests that show this works correctly, and these tests are pretty difficult to write so we can certainly help with that part, but see if you can try this out and if it's broken (as I bet it will be) take a crack at fixing it.

[VictorBezak]

Wonderful, thank you for the feedback @alexcjohnson. I'll start chipping away at this in my free-time and we'll keep the conversation going!

[VictorBezak]

Starting from scratch! Will comment with link to new PR when it is open

[VictorBezak]

I've been receiving help for the last 5 weeks from fellow developer, Filipe Santiago @filipesantiagoAM, and together we've managed to put together an MVP solution! This still needs some tests and approvals before being merged, but sharing the PR here for public visibility!

#6356