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

Issues Deferred from Time Series Redesign #356

Open
1 of 5 tasks
GabrielKS opened this issue Apr 26, 2024 · 1 comment
Open
1 of 5 tasks

Issues Deferred from Time Series Redesign #356

GabrielKS opened this issue Apr 26, 2024 · 1 comment

Comments

@GabrielKS
Copy link
Contributor

GabrielKS commented Apr 26, 2024

Here are a few issues that came up in #349 that didn't quite merit being dealt with there but should probably be addressed at some point. Some of these might be spun off into their own issues.

  • Currently, time series resolution is validated when adding a time series to a manager, and this consists of comparing the stated resolution to the difference between the first two timestamps. This sort of validation should instead be done in the time series constructor, and it should compare the differences between all consecutive timestamps, so it is never possible to construct an invalid time series. More radically, one might ask why we are storing this redundant information at all rather than having a getter that computes it. See Re-design time series management #349 (comment) and Re-design time series management #349 (comment)
  • Just a reminder that this needs to happen #360
  • We might want to check/enforce somewhere that features are not of mixed type. Or we might not. Current plan is to add a check for this in check_consistency and allow the user to override. See Re-design time series management #349 (comment)
  • We are using Dates.DateTime(0) as a null value, which could be problematic. This existed in the codebase before this PR so we haven't changed it yet. See Re-design time series management #349 (comment)
  • Deterministic Type Hierarchy #378: I don't love how we special-case (in this PR and in existing code) that Deterministic, a concrete type, sometimes also refers to DeterministicSingleTimeSeries, a different concrete type. I don't know about the typical user, but this definitely confused me before it was explained to me. We would have to think about how to resolve this without breaking existing code. For now we'll keep it as-is but at some point restore the behavior that you can pass abstract types to get_time_series; when that is done, there is a change that can be reverted in PSY. See Re-design time series management #349 (comment)
@jd-lara
Copy link
Member

jd-lara commented Apr 26, 2024

@GabrielKS why does using Dates.DateTime(0) as a null value problematic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants