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

Should Temporal vs. Sequence really be a property of the timeline itself? #8471

Open
teh-cmc opened this issue Dec 16, 2024 · 1 comment
Open
Labels
😤 annoying Something in the UI / SDK is annoying to use 🧑‍💻 dev experience developer experience (excluding CI) 💬 discussion 🪵 Log & send APIs Affects the user-facing API for all languages

Comments

@teh-cmc
Copy link
Member

teh-cmc commented Dec 16, 2024

Our timelines carry a name, a value, and an extra property indicating whether they are temporal (e.g. unix timestamps) or sequential (e.g. frame numbers).

This extra property has been the source of many, many bugs, and is still the source of a lot of complexity, both for us and for our end users.

For us, it is a constant struggle to try and carry that extra information everywhere timelines go, and make sure to never rely on a timeline name alone (which gets harder and harder as you go up in abstraction layers, e.g. the dataframe APIs). Otherwise it's effectively UB.

For end users, it is a pitfall that's very easy to fall into (e.g. using a timeline name as both sequential and temporal is UB).

My gut feeling is that this shouldn't even be a timeline property, but merely a viewer/blueprint setting (it's only a timestamp formatter after all, isn't it), which would vastly simplify everything.
You could imagine having heuristics to set that blueprint property based on a column tag on the timeline column.

Opinions?

@teh-cmc teh-cmc added 💬 discussion 😤 annoying Something in the UI / SDK is annoying to use 🧑‍💻 dev experience developer experience (excluding CI) 🪵 Log & send APIs Affects the user-facing API for all languages labels Dec 16, 2024
@jleibs
Copy link
Member

jleibs commented Dec 16, 2024

Temporal vs Sequence isn't governed by (our) metadata -- the fact that I included it in https://github.com/rerun-io/rerun/pull/8461/files wasn't even necessary.

It's actually determined by the underlying arrow datatypes:

  • Sequence uses pa.int64()
  • Temporal uses pa.timestamp("ns")

I think the issue that this is a source of bugs is still a valid issue, but it's not as easy as not tagging it -- we actually would need to drop support for pa.timestamp and encode all timestamps with ints, which doesn't feel right to me either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use 🧑‍💻 dev experience developer experience (excluding CI) 💬 discussion 🪵 Log & send APIs Affects the user-facing API for all languages
Projects
None yet
Development

No branches or pull requests

2 participants