enhanced the default datetimeformatter with a dynamic one

[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: fixes [FEATURE] datetime formatting: enhance default formatting #13186
• tests added / passed
• release document entry (if new feature or API change)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-3.7@fe06f34). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             branch-3.7   #13854   +/-   ##
=============================================
  Coverage              ?   93.17%           
=============================================
  Files                 ?      284           
  Lines                 ?    19965           
  Branches              ?        0           
=============================================
  Hits                  ?    18603           
  Misses                ?     1362           
  Partials              ?        0           

[bryevdv]

Unfortunately, this will take more work / different approach. The issue is that the way things are done in the PR, every single axis has a different DatetimeTickFormatter instance, but all of those share one single "global" DatetimeTickFormatter for their context. That won't work.

An immediate solution could be get rid of InstanceDefault and just pass the callable DYNAMIC_DATETIME_FORMATTER for the value. That would allow for every axis to get all new unique objects. But then the ref docs will suffer, because rendering these kinds of "dynamic" defaults better is what InstanceDefault was added for.

Or perhaps InstanceDefault can be made more sophisticated in some way, but that will require new development.

[muendlein]

@bryevdv Thanks for pointing out the sided effects. However I am wondering if there have been similar cases in the past or is this the first one?
As for moving forward, do you consider your proposed immediate solution as an viable option (given the downsides)?

[bryevdv]

It's not common to have dynamic defaults, less common still have to have dynamic defaults that are model instances, and among that tiny set, I do believe this would be the first time ever a dynamic instance default itself had property that was also such.

I think you should try building without InstanceDefault, and then also build the docs to see how badly things render in the reference guide. If it's just some opaque but short repr that shows up, that's probably fine. If it's some giant mess that takes up half the page looking like a C++ error message, that is probably not fine.

[muendlein]

@bryevdv Building without InstanceDefault works out fine. When building the docs there aren't any rendering issues but of course the static (and therefore wrong) default values are shown.

[bryevdv]

@muendlein did you expect to be able to pick this PR back up? If not I think we will close it. I think if the default values are just shown sub-optimally then we could proceed with a side issue to improve the docs tooling at some point.

[muendlein]

@bryevdv Thank you for the reminder, I have actually forgotten to push a commit. Regarding the docs, what locations shall I take a closer look at, apart from https://docs.bokeh.org/en/latest/docs/reference/models/axes.html#bokeh.models.DatetimeAxis (which looks good for me)?
If possible I would also like to get some feedback if the proposed defaults are okay.

[bryevdv]

@muendlein LGTM but let's also get input from the HV folks about defaults changes since they will see the impact downstream. cc @philippjfr @jbednar

[jbednar]

@muendlein LGTM but let's also get input from the HV folks about defaults changes since they will see the impact downstream. cc @philippjfr @jbednar

From the description and from what I can see in the code, the changes seem fine to me. Hopefully @philippjfr can report back about the actual behavior of the code in practice, at least once there is a dev release to test.

[bryevdv]

Quick check: is DYNAMIC_DATETIME_FORMATTER the best name? The base datetime formatter is already "dynamic" in the sense that it changes across scales. What sets this one apart specifically is that it provides context. Maybe DATETIME_CONTEXT_FORMATTER or CONTEXTUAL_DATETIME_FORMATTER? Or something else?

[muendlein]

Quick check: is DYNAMIC_DATETIME_FORMATTER the best name? The base datetime formatter is already "dynamic" in the sense that it changes across scales. What sets this one apart specifically is that it provides context. Maybe DATETIME_CONTEXT_FORMATTER or CONTEXTUAL_DATETIME_FORMATTER? Or something else?

Probably not the best name. I would also suggest DYNAMIC_CONTEXTUAL_DATETIME_FORMATTER even though it's quite long. Overall I don't have strong preference however.

[bryevdv]

@muendlein let's go with CONTEXTUAL_DATETIME_FORMATTER I think the "dynamic" part is implicit in all datetime formatters (and thus redundant in the name)

[bryevdv]

@philippjfr I will approve this in the next day or two absent any other comments

[philippjfr]

I too prefer the contextual prefix but otherwise I love this change.

[bryevdv]

Last thing @muendlein it would be good to add a line in 3.7.0.rst release notes (we are trying to be better about adding these in PRs rather than waiting to collect things at the last minute)

[bryevdv]

Not sure what is going awry with the codecov jobs but everything else is passing so I will just go ahead and merge this now. Thanks @muendlein !

[bryevdv]

@muendlein So one consequence of this change is that now examples that really just care about time of day look like this:

i.e. the y-axis show the context for 1970 which is not really appropriate. Do you have any thoughts? Can you open a new issue to discuss improvements of usage that needs to adapt?

[muendlein]

@bryevdv I have created a PR (#14397) to fix this by removing the context. One long term solution can be a formatter for time deltas (created an issue for that #14398).

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.