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

Add tickmode "proportional" #6827

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e7a2ffa
Add/calc tickmode "proportional" in `cartesian/`
ayjayt Dec 26, 2023
6d48a3b
Fix bug by which tickvals forever expands
ayjayt Dec 26, 2023
00e884a
Make tickmode proportional calculate reversed axes
ayjayt Dec 26, 2023
931199e
Use lib.simpleMap(); Refactor var names.
ayjayt Dec 26, 2023
fced9ce
Add log math to tickmode proportional
ayjayt Dec 26, 2023
b2abf08
Remove console debug messages
ayjayt Dec 26, 2023
910163e
Add description in layout_attributes.js
ayjayt Dec 26, 2023
e21af96
Fix linter errors
ayjayt Dec 26, 2023
ddfaad7
update plot-scheme diff
ayjayt Dec 26, 2023
d42dd73
Restore proportional array later in loop:
ayjayt Dec 27, 2023
5d7b1b9
Remove _isSet flag from tickVals before resetting
ayjayt Dec 28, 2023
db1f82e
Add parameterized tests:
ayjayt Dec 28, 2023
a2ac023
Fix tests to pass if #6828 is addressed
ayjayt Dec 28, 2023
1fedfc7
Clean up commenting/address lint errors.
ayjayt Dec 28, 2023
e8f2052
Fix a bit more lint
ayjayt Dec 28, 2023
c3f590d
Merge branch 'master' into pikul-proportional-ticks
ayjayt Jan 2, 2024
7e93370
Change tickmode 'proportional' to 'domain array'
ayjayt Jan 3, 2024
afba178
Add tickmode 'full domain'
ayjayt Jan 3, 2024
aca7a92
Set default so `full domain` doesn't set to `auto`
ayjayt Jan 3, 2024
195dfa5
Fix various errata/typos
ayjayt Jan 3, 2024
a788ccc
Add test for full-domain
ayjayt Jan 3, 2024
ecffc49
Merge branch 'master' into pikul-proportional-ticks
ayjayt Jan 4, 2024
3089c4a
Restructure parameterization and fix linting
ayjayt Jan 9, 2024
99d336a
Intermediate commit w/ partial work- new strat:
ayjayt Jan 12, 2024
4476d38
Merge branch 'master' into pikul-proportional-ticks
ayjayt Jan 12, 2024
3502d49
Revert branch to equal master
ayjayt Jan 12, 2024
09ebcfc
Lint and refactor
ayjayt Jan 12, 2024
3fe254b
Merge branch 'pikul-proportional-ticks-old' into pikul-proportional-t…
ayjayt Jan 12, 2024
eb82723
Revert completely to old tick algo
ayjayt Jan 12, 2024
fe6f8d0
update git-scheme diff
ayjayt Jan 12, 2024
b3956a0
Merge branch 'master' into pikul-proportional-ticks
ayjayt Jan 24, 2024
7176049
Remove category test in prep for disallowing
ayjayt Jan 25, 2024
15387ed
Refactor as to not override ax.tickvals:
ayjayt Jan 25, 2024
2ab0b41
Rework some if-then to golf mode (?:)
ayjayt Jan 25, 2024
f1a4db8
Lint
ayjayt Jan 25, 2024
ee17af2
Make 'auto' to function as 'array' when [] present
ayjayt Jan 25, 2024
93e4bb8
Refix incorrect patch ee17af22f393c
ayjayt Jan 25, 2024
61efdef
Merge branch 'master' into pikul-proportional-ticks
ayjayt Feb 9, 2024
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
6 changes: 5 additions & 1 deletion src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2945,7 +2945,11 @@ function getDiffFlags(oldContainer, newContainer, outerparts, opts) {
// so newContainer won't have them.
if((key === 'tick0' || key === 'dtick') && outerparts[0] !== 'geo') {
var tickMode = newContainer.tickmode;
if(tickMode === 'auto' || tickMode === 'array' || !tickMode) continue;
if(tickMode === 'auto' ||
tickMode === 'array' ||
tickMode === 'domain array' ||
tickMode === 'full domain' ||
!tickMode) continue;
}
// FIXME: Similarly for axis ranges for 3D
// contourcarpet doesn't HAVE zmin/zmax, they're just auto-added. It needs them.
Expand Down
83 changes: 78 additions & 5 deletions src/plots/cartesian/axes.js
Original file line number Diff line number Diff line change
Expand Up @@ -682,10 +682,13 @@ axes.prepTicks = function(ax, opts) {
if(ax._name === 'radialaxis') nt *= 2;
}

if(!(ax.minor && ax.minor.tickmode !== 'array')) {
if(!(ax.minor &&
(ax.minor.tickmode !== 'array' &&
ax.minor.tickmode !== 'domain array' &&
ax.minor.tickmode !== 'full domain'))) {
// add a couple of extra digits for filling in ticks when we
// have explicit tickvals without tick text
if(ax.tickmode === 'array') nt *= 100;
if(ax.tickmode === 'array' || ax.tickmode === 'domain array' || ax.tickmode === 'full domain') nt *= 100;
}

ax._roughDTick = Math.abs(rng[1] - rng[0]) / nt;
Expand Down Expand Up @@ -920,7 +923,12 @@ axes.calcTicks = function calcTicks(ax, opts) {
var minorTicks = [];

var tickVals = [];
var tickFractionalVals = [];
tickFractionalVals._isSet = false;

var minorTickVals = [];
var minorTickFractionalVals = [];
minorTickFractionalVals._isSet = false;

var hasMinor = ax.minor && (ax.minor.ticks || ax.minor.showgrid);

Expand All @@ -944,9 +952,61 @@ axes.calcTicks = function calcTicks(ax, opts) {
axes.prepTicks(mockAx, opts);
}

if(mockAx.tickmode === 'full domain') {
var nt = mockAx.nticks; // does mockAx have nitkcs?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var nt = mockAx.nticks; // does mockAx have nitkcs?
var nt = mockAx.nticks;

Yep, you ensured that in tick_value_defaults, then (for minor ticks) this gets pulled into mockAx by the extendFlat.

if(nt === undefined) nt = 0;
if(nt === 0) {
// pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

For tickmode: 'auto', nticks: 0 or undefined is "super automatic":

if(!nt) {
if(ax.type === 'category' || ax.type === 'multicategory') {
minPx = ax.tickfont ? Lib.bigFont(ax.tickfont.size || 12) : 15;
nt = ax._length / minPx;
} else {
minPx = ax._id.charAt(0) === 'y' ? 40 : 80;
nt = Lib.constrain(ax._length / minPx, 4, 9) + 1;
}
// radial axes span half their domain,
// multiply nticks value by two to get correct number of auto ticks.
if(ax._name === 'radialaxis') nt *= 2;
}

ie ending up somewhere between 5 and 10 depending on the graph size. For full domain I wouldn't want us dividing the axis in 7ths though, so maybe just set it to 5 or something? If you really want no ticks just hide them 😉

This also reminds me: the new tickmodes should not be allowed at all on category or multicategory axes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Silent fail or throw error if they tried to put tickmode in category or multicategory?

} else if(nt === 1) {
tickVals = [0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tickVals = [0];
tickVals = [0.5];

If you just want one tick I'd put it in the middle. Not that it matters much, this is horrible whatever we choose since you can't tell the scale at all.

} else if(nt === 2) {
tickVals = [0, 1];
} else {
var increment = 1 / (nt - 1); // (nt-2) + 1
tickVals.push(0);
for(var tickIndex = 0; tickIndex < nt - 2; tickIndex++) {
tickVals.push((tickIndex + 1) * increment);
}
tickVals.push(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For major ticks this is good.

For minor, with the existing 'tickmode: 'auto' we take the calculated major ticks, divide the intervals by minor.nticks, and draw minor ticks everywhere except where there are already major ticks. Which means with minor.nticks=5 we actually draw 4 minor ticks in each major interval. That's a little confusing compared with what we're doing here with the new 'full domain' major ticks but I don't see a better design so let's keep major as is, and have minor behave in the analogous way: when major and minor are both 'full domain' we should draw minor.nticks - 1 ticks in each major tick interval.

What about other major/minor tickmode combinations? I think it'd be reasonable to restrict the allowed minor.tickmode based on major tickmode so that at least the major and minor ticks always move together:

  • tickmode in 'auto', 'linear', 'array' -> minor.tickmode also in 'auto', 'linear', 'array'. We currently allow tickmode: 'array', minor.tickmode: 'auto' but this seems like a bug to me as the minor ticks don't (and can't!) evenly divide major tick intervals, and the spacing of minor ticks depends on minor.nticks but in some strange way. @archmoj was this intentional or should we prohibit minor.tickmode: 'auto' with tickmode: 'array'?
  • tickmode: 'sync' -> minor ticks should match the minor ticks on the sync'd axis, which they currently almost do, but we get some extras so this is a bug (cc @archmoj - try opening the new_tickmode_sync mock and calling Plotly.relayout(gd,{'yaxis2.minor.ticks':'outside'}) - you'll see the correct minor ticks plus 11 extras, some of which coincide with the expected ticks, some don't, you can see them all if you drag only one of the y axes). Seems to me you should be able to set the visibility and appearance of minor ticks/grid on this axis, but their positions should be sync'd to the positions of minor ticks on the main axis
  • tickmode in 'full domain', 'domain array' -> minor.tickmode also in 'full domain', 'domain array'. But per my comment above I think with tickmode: 'domain array' we should only accept minor.tickmode: 'domain array' too, or no minor ticks.

Copy link
Contributor Author

@ayjayt ayjayt Jan 24, 2024

Choose a reason for hiding this comment

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

I'll take a look at getting minor ticks spaced out between major ticks. Also since I'm working on it I'll take a look at some of the other stuff.

Does plotly throw/raise errors on invalid combinations, or is silent?

}
if(major) {
Lib.nestedProperty(ax, 'tickvals').set(tickVals);
} else {
Lib.nestedProperty(ax.minor, 'tickvals').set(tickVals);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to avoid altering real attribute values after the supplyDefaults step. But you can set an underscored attribute like _tickvals if you like, then you just need to look for that in arrayTicks (if you're in one of the domain tickmodes). That should avoid the need to reset it later too, I'd imagine.

Also kudos on figuring out nestedProperty - but it's overkill here since we know the attribute names, you can just do ax._tickvals = tickVals etc - or if you feel like code golf, (major ? ax : ax.minor)._tickvals = tickVals 😎

}
}
// tickmode 'domain array' is just 'array' but with a pre-calc step
// original comment:
// now that we've figured out the auto values for formatting
// in case we're missing some ticktext, we can break out for array ticks
if(mockAx.tickmode === 'array') {
if(mockAx.tickmode === 'array' || mockAx.tickmode === 'domain array' || mockAx.tickmode === 'full domain') {
// Mapping proportions to array:
if(mockAx.tickmode === 'domain array' || mockAx.tickmode === 'full domain') {
var width = (maxRange - minRange);
if(axrev) width *= -1;
var offset = !axrev ? minRange : maxRange;

var currentFractionalVals = [];
var currentValsProp;
if(major) {
currentValsProp = Lib.nestedProperty(ax, 'tickvals'); // Do we need this?
currentFractionalVals = tickFractionalVals = currentValsProp.get();
tickFractionalVals._isSet = true;
} else {
currentValsProp = Lib.nestedProperty(ax.minor, 'tickvals');
currentFractionalVals = minorTickFractionalVals = currentValsProp.get();
minorTickFractionalVals._isSet = true;
}

var mappedVals = Lib.simpleMap(currentFractionalVals,
function(fraction, offset, width, type) {
var mapped = offset + (width * fraction);
return (type === 'log') ? Math.pow(10, mapped) : mapped;
}, offset, width, type);
currentValsProp.set(mappedVals);
}

// Original 'array' only code
if(major) {
tickVals = [];
ticksOut = arrayTicks(ax, !isMinor);
Expand Down Expand Up @@ -1204,6 +1264,7 @@ axes.calcTicks = function calcTicks(ax, opts) {
ticksOut.push(t);
}
}

ticksOut = ticksOut.concat(minorTicks);

ax._inCalcTicks = false;
Expand All @@ -1213,6 +1274,18 @@ axes.calcTicks = function calcTicks(ax, opts) {
ticksOut[0].noTick = true;
}

// Reset tickvals back to domain array
if(tickFractionalVals._isSet) {
delete tickFractionalVals._isSet;
if(ax.tickmode === 'full domain') tickFractionalVals = [];
Lib.nestedProperty(ax, 'tickvals').set(tickFractionalVals);
}
if(minorTickFractionalVals._isSet) {
delete tickFractionalVals._isSet;
if(ax.minor.tickmode === 'full domain') tickFractionalVals = [];
Lib.nestedProperty(ax.minor, 'tickvals').set(minorTickFractionalVals);
}

return ticksOut;
};

Expand Down Expand Up @@ -1617,7 +1690,7 @@ axes.tickFirst = function(ax, opts) {
// more precision for hovertext
axes.tickText = function(ax, x, hover, noSuffixPrefix) {
var out = tickTextObj(ax, x);
var arrayMode = ax.tickmode === 'array';
var arrayMode = (ax.tickmode === 'array' || ax.tickmode === 'domain array' || ax.tickmode === 'full domain');
var extraPrecision = hover || arrayMode;
var axType = ax.type;
// TODO multicategory, if we allow ticktext / tickvals
Expand Down Expand Up @@ -3333,7 +3406,7 @@ axes.drawGrid = function(gd, ax, opts) {

var counterAx = opts.counterAxis;
if(counterAx && axes.shouldShowZeroLine(gd, ax, counterAx)) {
var isArrayMode = ax.tickmode === 'array';
var isArrayMode = (ax.tickmode === 'array' || ax.tickmode === 'domain array' || ax.tickmode === 'full domain');
for(var i = 0; i < majorVals.length; i++) {
var xi = majorVals[i].x;
if(isArrayMode ? !xi : (Math.abs(xi) < ax.dtick / 100)) {
Expand Down
15 changes: 11 additions & 4 deletions src/plots/cartesian/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ var DAY_OF_WEEK = constants.WEEKDAY_PATTERN;

var minorTickmode = {
valType: 'enumerated',
values: ['auto', 'linear', 'array'],
values: ['auto', 'linear', 'array', 'domain array', 'full domain'],
editType: 'ticks',
impliedEdits: {tick0: undefined, dtick: undefined},
description: [
Expand All @@ -23,9 +23,16 @@ var minorTickmode = {
'If *linear*, the placement of the ticks is determined by',
'a starting position `tick0` and a tick step `dtick`',
'(*linear* is the default value if `tick0` and `dtick` are provided).',
'If *array*, the placement of the ticks is set via `tickvals`',
'and the tick text is `ticktext`.',
'(*array* is the default value if `tickvals` is provided).'
'If *array*, the placement of the ticks is set via `tickvals`,',
'which are actual values, and the tick text is `ticktext`.',
'(*array* is the default value if `tickvals` is provided).',
'If *full domain*, the number of ticks is set bia `nticks` but ticks',
'are placed first at both axis ends and then at equal proportions',
'between the axis. So `nticks=5` would put ticks at both ends and',
'every quarter.',
'If *domain array*, the placement is similiar to *array* except that',
'`tickvals` are fractions between 0 and 1 representing distance on',
'the corresponding axis.'
].join(' ')
};

Expand Down
3 changes: 1 addition & 2 deletions src/plots/cartesian/tick_value_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ module.exports = function handleTickValueDefaults(containerIn, containerOut, coe
_dtick ? 'linear' :
'auto';
var tickmode = coerce(prefix + 'tickmode', tickmodeDefault);

if(tickmode === 'auto' || tickmode === 'sync') {
if(tickmode === 'auto' || tickmode === 'sync' || tickmode === 'full domain') {
coerce(prefix + 'nticks');
} else if(tickmode === 'linear') {
// dtick is usually a positive number, but there are some
Expand Down
Loading