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 'tickformatstops' #1965

Merged
merged 17 commits into from Oct 16, 2017

Conversation

Projects
None yet
4 participants
@apalchys
Contributor

apalchys commented Aug 22, 2017

Related to #1946

This PR adds tickformatstops axis config attribute. The purpose of new attribute is to help customize formatting of axis ticks by dtick value aka "zoom level". The following types of axis support tickformatstops:

  • date
  • linear

Usage example:

tickformatstops:
[
    {
        dtickrange: [null, 1000],
        value: "%H:%M:%S.%L"
    },
    {
        dtickrange: [1000, 60000],
        value: "%H:%M:%S"
    }
]

dtickrange should be specified using 2-value array with dtick values.
Also it is possible to use null as value if start/end of range is not defined. In this case it will be interpred as absolute mix/max value.
The value should be valid tickformat string

I will add tests later if the implementation looks good.

Question to discuss:

  • If ranges overlaps, the code takes max applicable range. For example we have the following dtickranges

    #1 [86400000, "M1"]
    #2 ["M1", "M12"]
    #3 ["M12", null]

M1 is specified in ranges #1 and #2 and the code will apply a format string associated with range #2.
Is it ok?

  • Do we need to support log axis?
@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Aug 22, 2017

Member

Looking solid so far! Thanks very much @apalchys 🎉

If ranges overlaps, the code takes max applicable range

At first glance, that sounds right to me. 👍

Do we need to support log axis

Yes please!

I will add tests later if the implementation looks good.

😏

Member

etpinard commented Aug 22, 2017

Looking solid so far! Thanks very much @apalchys 🎉

If ranges overlaps, the code takes max applicable range

At first glance, that sounds right to me. 👍

Do we need to support log axis

Yes please!

I will add tests later if the implementation looks good.

😏

@talgalili

This comment has been minimized.

Show comment
Hide comment
@talgalili

talgalili Aug 22, 2017

Thanks @apalchys - I'm very excited about this feature being added.

talgalili commented Aug 22, 2017

Thanks @apalchys - I'm very excited about this feature being added.

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Aug 23, 2017

Contributor

Added support for log axis

Contributor

apalchys commented Aug 23, 2017

Added support for log axis

coerce('dtickrange');
coerce('value');
valuesOut.push(valueOut);

This comment has been minimized.

@etpinard

etpinard Aug 23, 2017

Member

Nicely done. Thanks!

🎉

@etpinard

etpinard Aug 23, 2017

Member

Nicely done. Thanks!

🎉

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Aug 28, 2017

Contributor

added tests

Contributor

apalchys commented Aug 28, 2017

added tests

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Aug 28, 2017

Member

added tests

Great. Are you planning on addressing #1965 (comment) ?

Member

etpinard commented Aug 28, 2017

added tests

Great. Are you planning on addressing #1965 (comment) ?

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Aug 28, 2017

Contributor

Great. Are you planning on addressing #1965 (comment) ?

Yep. I will try to fix everything by tomorrow.

Contributor

apalchys commented Aug 28, 2017

Great. Are you planning on addressing #1965 (comment) ?

Yep. I will try to fix everything by tomorrow.

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Sep 5, 2017

Contributor

moving discussion to the root level from #1965 (comment)

the ability to have something like nticks to be respected when tickmode='array'

It seems I am stuck here and not sure what to do next :)
After re-reading issues, it seems we needs something like this:

[{
    deltarange:  [40, 30]
    nticks: 50
},
{
    deltarange:  [800, 1200]
    nticks: 5
}]

where deltarange is a range of delta between min and max current range values (https://github.com/plotly/plotly.js/blob/master/src/plots/cartesian/axes.js#L268). In this case we can configure to display fewer ticks if we zoom out.

Also for me adding a special logic for category axis to tickformatstops (where we use dticks) looks a little bit tricky. Maybe it makes sense to move it to a separate property?

Contributor

apalchys commented Sep 5, 2017

moving discussion to the root level from #1965 (comment)

the ability to have something like nticks to be respected when tickmode='array'

It seems I am stuck here and not sure what to do next :)
After re-reading issues, it seems we needs something like this:

[{
    deltarange:  [40, 30]
    nticks: 50
},
{
    deltarange:  [800, 1200]
    nticks: 5
}]

where deltarange is a range of delta between min and max current range values (https://github.com/plotly/plotly.js/blob/master/src/plots/cartesian/axes.js#L268). In this case we can configure to display fewer ticks if we zoom out.

Also for me adding a special logic for category axis to tickformatstops (where we use dticks) looks a little bit tricky. Maybe it makes sense to move it to a separate property?

@talgalili

This comment has been minimized.

Show comment
Hide comment
@talgalili

talgalili Sep 5, 2017

@apalchys who (in this repo) do you think could help you solve this?
(i.e.: to gain "the ability to have something like nticks to be respected when tickmode='array' ")

talgalili commented Sep 5, 2017

@apalchys who (in this repo) do you think could help you solve this?
(i.e.: to gain "the ability to have something like nticks to be respected when tickmode='array' ")

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Sep 5, 2017

Contributor

@talgalili not sure I know the answer to your question. it is my first PR and @etpinard is my first reviewer.

Contributor

apalchys commented Sep 5, 2017

@talgalili not sure I know the answer to your question. it is my first PR and @etpinard is my first reviewer.

@alexcjohnson

This comment has been minimized.

Show comment
Hide comment
@alexcjohnson

alexcjohnson Oct 9, 2017

Contributor

@apalchys apologies for letting this languish. I'm looking at the open pieces now. In merging master it looks like the issue you encountered with strict_d3 was also solved independently by #1994 - looked to me as though they both would work fine, I elected to keep #1994 but with your added comment. Does it still work correctly for you?

Contributor

alexcjohnson commented Oct 9, 2017

@apalchys apologies for letting this languish. I'm looking at the open pieces now. In merging master it looks like the issue you encountered with strict_d3 was also solved independently by #1994 - looked to me as though they both would work fine, I elected to keep #1994 but with your added comment. Does it still work correctly for you?

@alexcjohnson

This comment has been minimized.

Show comment
Hide comment
@alexcjohnson

alexcjohnson Oct 10, 2017

Contributor

If ranges overlaps, the code takes max applicable range

I would actually prefer to take the first matching entry. Seems to me this is easier for users to parse, easier and more flexible to generate (you can start from the lowest and specify [null, max] for each of the dtickrange settings if you want, or start from the top and specify [min, null]) and more efficient in our code (we can bail out of the loop at the first match).

Contributor

alexcjohnson commented Oct 10, 2017

If ranges overlaps, the code takes max applicable range

I would actually prefer to take the first matching entry. Seems to me this is easier for users to parse, easier and more flexible to generate (you can start from the lowest and specify [null, max] for each of the dtickrange settings if you want, or start from the top and specify [min, null]) and more efficient in our code (we can bail out of the loop at the first match).

@alexcjohnson

This comment has been minimized.

Show comment
Hide comment
@alexcjohnson

alexcjohnson Oct 10, 2017

Contributor

TBH I'm not sure what the goal is with adding category axes here.

  • category axes DO use dtick (see eg category_dtick_3 that has enough categories we don't show them all, we automatically drop to every third one)
  • category axes DO NOT use tickformat - what would it do? Categories are already explicit strings. so tickformatstops also seems to me not to apply.
  • tickvals and ticktext can be used with any axis type, but it would frankly be kind of weird to use them with category, why wouldn't you just use the label you want as the category? They're normally for putting specific ticks on other axis types.

@talgalili mentioned:

the ability to have something like nticks to be respected when tickmode='array'

That to me seems like a separate issue from what we're dealing with here. This PR is about how to format the ticks, not about which ticks to show. There's another long-neglected PR #303 that may be what you're interested in for this purpose: setting a minimum pixel distance between ticks. I think that's probably the only way you can solve this for the tickmode='array' case: nothing is guaranteed about their spacing otherwise so solutions like "display every nth tick" wouldn't work, but "iterate through the array and display each tick iff it's more than n pixels away from all the others we've already drawn" would work. @talgalili do you want to take this part of the discussion over there and see if we can revive that PR? Plotly staff may not be able to get to it any time soon though, but maybe someone else wants to pitch in...

Contributor

alexcjohnson commented Oct 10, 2017

TBH I'm not sure what the goal is with adding category axes here.

  • category axes DO use dtick (see eg category_dtick_3 that has enough categories we don't show them all, we automatically drop to every third one)
  • category axes DO NOT use tickformat - what would it do? Categories are already explicit strings. so tickformatstops also seems to me not to apply.
  • tickvals and ticktext can be used with any axis type, but it would frankly be kind of weird to use them with category, why wouldn't you just use the label you want as the category? They're normally for putting specific ticks on other axis types.

@talgalili mentioned:

the ability to have something like nticks to be respected when tickmode='array'

That to me seems like a separate issue from what we're dealing with here. This PR is about how to format the ticks, not about which ticks to show. There's another long-neglected PR #303 that may be what you're interested in for this purpose: setting a minimum pixel distance between ticks. I think that's probably the only way you can solve this for the tickmode='array' case: nothing is guaranteed about their spacing otherwise so solutions like "display every nth tick" wouldn't work, but "iterate through the array and display each tick iff it's more than n pixels away from all the others we've already drawn" would work. @talgalili do you want to take this part of the discussion over there and see if we can revive that PR? Plotly staff may not be able to get to it any time soon though, but maybe someone else wants to pitch in...

@apalchys

This comment has been minimized.

Show comment
Hide comment
@apalchys

apalchys Oct 10, 2017

Contributor

I elected to keep #1994 but with your added comment. Does it still work correctly for you?

Yes, it does. Thanks.

I would actually prefer to take the first matching entry

Updated

Contributor

apalchys commented Oct 10, 2017

I elected to keep #1994 but with your added comment. Does it still work correctly for you?

Yes, it does. Thanks.

I would actually prefer to take the first matching entry

Updated

@alexcjohnson

This comment has been minimized.

Show comment
Hide comment
@alexcjohnson

alexcjohnson Oct 10, 2017

Contributor

@apalchys great, thanks for the changes! I linted and updated the baseline image for the new "first" logic. I think this is ready to go (assuming tests pass this time - ignore appveyor) but we're going to wait a few days to merge to master, so we can give v1.31 any bugfixes it needs before adding features (which will go in v1.32.0)

Contributor

alexcjohnson commented Oct 10, 2017

@apalchys great, thanks for the changes! I linted and updated the baseline image for the new "first" logic. I think this is ready to go (assuming tests pass this time - ignore appveyor) but we're going to wait a few days to merge to master, so we can give v1.31 any bugfixes it needs before adding features (which will go in v1.32.0)

},
value: {
valType: 'string',
dflt: '',

This comment has been minimized.

@etpinard

etpinard Oct 12, 2017

Member

Is axis.tickformat: '' a valid format? More precisely, could

tickformatstop: [{
  dtickrange: [/* */],
  value: ''
}]

be used to hide tick labels for some dtick range?

@etpinard

etpinard Oct 12, 2017

Member

Is axis.tickformat: '' a valid format? More precisely, could

tickformatstop: [{
  dtickrange: [/* */],
  value: ''
}]

be used to hide tick labels for some dtick range?

This comment has been minimized.

@alexcjohnson

alexcjohnson Oct 12, 2017

Contributor

no, tickformat: '' gives our default formatting. I suppose we could imagine making our own "hide" tickformat (for date axes you can already hack this with tickformat: ' ' but that doesn't work for numbers) but it seems a bit of a kludgy way to handle a very esoteric edge case.

@alexcjohnson

alexcjohnson Oct 12, 2017

Contributor

no, tickformat: '' gives our default formatting. I suppose we could imagine making our own "hide" tickformat (for date axes you can already hack this with tickformat: ' ' but that doesn't work for numbers) but it seems a bit of a kludgy way to handle a very esoteric edge case.

@etpinard

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Oct 16, 2017

Member

1.31.1 is out, 💃

Member

etpinard commented Oct 16, 2017

1.31.1 is out, 💃

@alexcjohnson alexcjohnson merged commit cad8e06 into plotly:master Oct 16, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment