Skip to content

Range break semantics #4655

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

Closed
nicolaskruchten opened this issue Mar 17, 2020 · 14 comments
Closed

Range break semantics #4655

nicolaskruchten opened this issue Mar 17, 2020 · 14 comments
Assignees
Labels
bug something broken feature something new
Milestone

Comments

@nicolaskruchten
Copy link
Contributor

Per Slack, we want to have three modes of operation, after #4653 is merged:

  • pattern="day of week"/bounds: in this mode, we accept only integer values in bounds, and operation is always []
  • pattern="hour"/bounds/operation: in this mode we accept floats in bounds and operation is honoured for points at the boundaries
  • values/operation: in this mode we accept ISO datetime values and operation is honoured for points at the boundaries
@nicolaskruchten
Copy link
Contributor Author

@alexcjohnson you've thought about this more than I have... can we unify the behaviour of operation even in day-of-week mode somehow with respect to points on the boundaries?

@nicolaskruchten nicolaskruchten added this to the v1.53.0 milestone Mar 17, 2020
@archmoj
Copy link
Contributor

archmoj commented Mar 17, 2020

  • pattern="hour"/bounds/operation: in this mode we accept floats in bounds and operation is honoured for points at the boundaries

Could you please provide an input+output example for this case?

@archmoj
Copy link
Contributor

archmoj commented Mar 17, 2020

  • values/operation: in this mode we accept ISO datetime values and operation is honoured for points at the boundaries

To be "honoured", should the points be included in the plot? i.e. excluded from the rangebreak

@nicolaskruchten
Copy link
Contributor Author

For "hour" and "values" modes I'm just describing the current behaviour I believe.

@archmoj
Copy link
Contributor

archmoj commented Mar 17, 2020

Regarding the first case,
Another option may be

pattern: 'day', bounds: ['Sat', 'Sun']

instead of

pattern: 'day of week', bounds: [6, 0]

@alexcjohnson
Copy link
Collaborator

Yes, for 'hour' and 'values' modes @nicolaskruchten has, I believe, described the status quo accurately, and we can keep that.

Right now in 'day of week', after we use operation to determine which days to remove (which I think we can all agree at this point we're no longer going to do, but keep reading because I have some concerns about what we should do instead), the boundary behavior is what in the other modes would have been operation='[)' - that is, the left edge of the break range, ie Saturday at 00:00:00.000, is "within the break" so a data point there will not be drawn, but the right edge of the break range, Monday at 00:00:00.000 will be drawn. So if you give this data:
Friday 23:59:59.998, Friday 23:59:59.999, Monday 00:00:00.000, Monday 00:00:00.001
The result will be four equally spaced points, no values between that last Friday and the first Monday point would be allowed, and no two points with different data values would ever share the same screen position.

This seems to me like it will work well in the short term for the two main cases I can think of:

  • Timestamped data, with or without additional (overnight) hour breaks. If we make the breaks visible, either via a gap in the axis line or a line across the plot at the break position, they're located in the correct location with no additional tweaking needed.
  • Whole-day data, ie data points located at 00:00:00 - you will see evenly spaced marks & ticks for M,T,W,Th,F,M,T,W... But this may become a problem when we insert of visible gap at the weekend: that mark will occur exactly at the Monday data point, whereas it should really be halfway between Friday and Monday. Two solutions I can see to this:
    1. Allow shifting the display of points and tick labels to the middle of the day, while leaving the break at midday as we have it. We've already been discussing this elsewhere anyway, though I don't see an issue...
    2. Allow shifting the break itself to halfway (or some other fraction) between days. This could probably be added easily as an option to the break definition, but it can't be the default (due to the timestamped case) This feels like a messy hack compared to (1), though maybe there's a case where someone wants a break from 4AM Saturday to 8AM Monday or some similar? Factory monitoring with graveyard shifts but not working through the weekend?

Then is there still a use case for doing something with operation when pattern='day of week'? For whole-day data I don't see it. For timestamped data possibly, but that would be a small subset of cases where you want the break not exactly midnight to midnight. I guess I don't see a harm in enabling it to work as it does with hours and values, as long as the default becomes [), at least for day of week and perhaps for all of them... but I feel like to get something out quickly we should just disable operation with day of week and come back to it if someone complains.

Re: ['Sat', 'Sun'] vs [6, 0] - I do like the clarity of the text version, and I see in an off-repo chat you've discussed allowing a case-insensitive first-three-letters, and defaulting to pattern='day of week' when bounds has text. That's quite nice. If we also keep the number format, we can imagine extending the number format to support fractions. That gets weird if you look at it too closely though - [6, 0] in fact means "remove from the first instant of Saturday to the last instant of Sunday," which is a period of 2 days even though the range you've specified - the difference between the two numbers, modulo 7 - is only 1 day. I recognize that it would look weird, but to be more precise about what we're doing it might be better actually to use ['Sat', 'Mon'] or [6, 1] to denote removing the weekend. Then if you wanted to have a break like I described above (4AM Saturday to 8AM Monday) you could specify it as [6.1667, 1.3333] or we could support a syntax like ['Sat 04:00', 'Mon 08:00'] and ['Sat', 'Mon'] would be equivalent to ['Sat 00:00', 'Mon 00:00']

As an aside: operation='()' feel sketchy to me, as you get two points with exactly the same position but different data values: https://codepen.io/alexcjohnson/pen/ZEGRQoZ
Screen Shot 2020-03-17 at 1 03 01 PM
I might argue for removing this option entirely.

Also there's a bug if you cross a break with operation='()' but have no further range https://codepen.io/alexcjohnson/pen/ExjRPea - seems like removing () would fix this, though if you use (] the range is weird, the one visible point is squished against the right edge of the plot, whereas [) for the same data puts the point in the middle where it belongs.

@nicolaskruchten
Copy link
Contributor Author

Update: per Slack, let’s get rid of operation and have the behaviour be what [) would have been.

@archmoj
Copy link
Contributor

archmoj commented Mar 19, 2020

Now concerning the third item i.e. when using values the behaviour is not correct: demo.

@alexcjohnson
Copy link
Collaborator

I think that's correct actually, because of dvalue which defaults to one day.

@alexcjohnson
Copy link
Collaborator

There's a hover issue in that codepen though - the first bar after the break only gets hover on its right half. I suspect this is because the left half of this bar is actually on the other side of the break, as far as the axis itself is concerned. I guess this is an early manifestation of the problems I was describing above when we get around to drawing the break on the axis and/or on the plot.

Switching to bounds instead of values this issue is still present.

My guess is this one will be fairly tough to fix...

@archmoj
Copy link
Contributor

archmoj commented Mar 19, 2020

They are still some hover issues with & without bars namely with reversed ranges 😕

@archmoj
Copy link
Contributor

archmoj commented Mar 19, 2020

I think that's correct actually, because of dvalue which defaults to one day.

Instead of milliseconds, isn't it better to have the dflt = 1 here?

dvalue: {
// TODO could become 'any' to add support for 'months', 'years'
valType: 'number',
role: 'info',
editType: 'calc',
min: 0,
dflt: ONEDAY,
description: [
'Sets the size of each `values` item.',
'The default is one day in milliseconds.'
].join(' ')
},

meaning 1 day? (or even better 1 hour to simplify the math for users)?

@alexcjohnson
Copy link
Collaborator

When we enable non-date axes, the default should be 1 for those cases; but on date axes we should keep numbers referring to milliseconds for consistency, until such time as we enable some sort of date interval units and we can explicitly specify this as "one day".

@archmoj
Copy link
Contributor

archmoj commented Mar 19, 2020

OK. Then we may close this issue.

@archmoj archmoj added bug something broken feature something new labels Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

No branches or pull requests

3 participants