Add scale up boundary to datetimetickformatter

[muendlein]

All pull requests must have an associated issue in the issue tracker. If there
isn't one, please go open an issue describing the defect, deficiency or desired
feature. You can read more about our issue and PR processes in the
wiki.

• issues: adresses [FEATURE] datetime formatting: enhance default formatting #13186
• tests added / passed
• release document entry (if new feature or API change)

[bryevdv]

@tcmetzger do you have any thoughts on the property name here? Offhand, scale_up_boundary seems a bit clunky to me.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.65%. Comparing base (04970ea) to head (f123b14).
Report is 6 commits behind head on branch-3.5.

Additional details and impacted files

@@             Coverage Diff             @@
##           branch-3.5   #13467   +/-   ##
===========================================
  Coverage       92.65%   92.65%           
===========================================
  Files             326      326           
  Lines           20733    20734    +1     
===========================================
+ Hits            19210    19211    +1     
  Misses           1523     1523           

[bryevdv]

I think this test needs a new name, up above there was a "should handle boolean" because the property strip_leading_zeros can accept boolean values and numeric values, and that test checked that the boolean case worked. But here scale_up_boundary is boolean flag, it only accepts booleans in any case. This test is checking one of the two cases specifically.

[muendlein]

Agreed. I will update this as soon as we agreed on a suitable property name.

[tcmetzger]

@tcmetzger do you have any thoughts on the property name here? Offhand, scale_up_boundary seems a bit clunky to me.

I see - especially since it refers to a boolean (when I read scale_up_boundary for the first time, I thought this was a parameter to define a boundary value after which things should be scaled up).

Some ideas:

• unit_scaling
• scale_units
• boundary_scaling
• unit_auto_scale

I think we don't necessarily need to include the "up" part, since it seems like any scaling that can be activated or deactivated is always up (i.e. there is no corresponding property that would activate DOWNscaling to the next lowest resolution, so no need to differentiate).

This is based on my understanding of the current code that implies that automatic scaling happens only if this property is set to True (and nothing happens if False). This might change based on Bryan's comment https://github.com/bokeh/bokeh/pull/13467/files#r1366182425, if I understand correctly!

[muendlein]

@bryevdv Any update on your review comment above?

[bryevdv]

@muendlein sorry not yet, it's a busy time of year. In the mean time, do you have an preference on the property name suggesstions?

[muendlein]

@muendlein sorry not yet, it's a busy time of year. In the mean time, do you have an preference on the property name suggesstions?

@bryevdv Take your time! I took some inspiration from the suggestions and my current favorite is boundary_auto_scale

[bryevdv]

@muendlein a couple of comments:

• looks like some conflicts have crept in, you probably want to merge/rebase on latests
• I still don't understand why the logic change is the correct one. Can you post some screenshots that demonstrate how things behave with it on/off?

Otherwise, I am also still unsure about the property and type. I am worried that making it a Bool will un-necessarily box us into a corner. What if other users want to scale up two levels on the boundary? Or always want a fixed level or the highest level? If we start of with Bool we severely limit future options in case new requests come up

[muendlein]

@bryevdv Thank you for your feedback!
I did a rebase, so the code should be usable again. Regarding the logic you can find an example below.
As for the type I am fully open to use an enum type which should allow for more flexibility later on. I would suggest using none and max to represent the current False and True values.

Scaling = True (current default behavior)

Scaling = False

[bryevdv]

@muendlein apologies, I just realized I had understood the issue and PR backwards—every other request previously has been to add more context, I did not realize that this is essentially the opposite. The tick formatter changes all make sense to me now.

And I actually think I am fine keeping this a boolean, too. If we did ever want to make it more configurable, we could add a second property that specifies "how" the scale up should be done.

@mattpap I'll plan to merge this tomorrow soon unless you want a look.

[bryevdv]

As for the name, I think I actually would advise to go with @tcmetzger's suggest of boundary_scaling

[mattpap]

I don't have comments on the logic of this change, but I made a quick tweak to get rid of ! type operator.

[bryevdv]

Thanks @muendlein !