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

Add user-defined upsamling resolution to time sync model #354

Closed
jmcook1186 opened this issue Dec 22, 2023 · 7 comments · Fixed by #990
Closed

Add user-defined upsamling resolution to time sync model #354

jmcook1186 opened this issue Dec 22, 2023 · 7 comments · Fixed by #990
Assignees
Labels
good-first-issue This issue is a good one for someone looking to get involved with this project/initiative.

Comments

@jmcook1186
Copy link
Contributor

Story

As a user I want IF to run as fast as possible.

Rationale

The time sync model currently works by upsampling all time series to 1 second resolution everywhere before resampling to the desired interval. This probably creates some unnecessary work as the upsampling could be done at a coarser resolution in many cases, as long as the upsampling resolution is finer than the initial () resolution of the input data
pre-normalization and the desired interval.

Implementation details

Identify where we have hardcoded the upsampling resolution to 1 second and swap out for some custom logic, likely derived from a resolution parameter passed in model config.

Priority

2/5
The important thing is that the model works well. It is important, but not urgent, to add this optimization.

Scope

Affects time sync model

Size

1 -2 days

What does "done" look like?

Model runs with user defined upsampling resolution

Does this require updates to documentation or other materials??

Yes, model documentation will need updating.

What testing is required?

Unit tests have 100% coverage and 100% pass rate.

Is this a known/expected update?

Discussed between core devs during time sync model building

@jmcook1186 jmcook1186 added this to the Carbon Hack milestone Dec 22, 2023
@jmcook1186 jmcook1186 added this to IF Dec 22, 2023
@jmcook1186 jmcook1186 moved this to Backlog in IF Dec 22, 2023
@jmcook1186 jmcook1186 moved this from Backlog to Ready in IF Dec 31, 2023
@jmcook1186 jmcook1186 removed the plugin label Mar 20, 2024
@jmcook1186 jmcook1186 moved this from Ready to Parked in IF Apr 25, 2024
@zanete zanete removed this from the Carbon Hack milestone Apr 29, 2024
@zanete zanete added good-first-issue This issue is a good one for someone looking to get involved with this project/initiative. and removed backlogged labels Jun 5, 2024
@mouhamadalmounayar
Copy link
Contributor

Hi @jmcook1186
I have some questions regarding this issue.

Given a simple input array :

[
  {
    "timestamp": "2023-12-12T00:00:00.000Z",
    "duration": 10,
    "carbon": 10
  }
]

with this basic config :

{
  "start-time": "2023-12-12T00:00:00.000Z",
  "end-time": "2023-12-12T00:00:10.000Z",
  "interval": 5,
  "allow-padding": true
}

the TimeSync Model currently upsamples each observation from the input array to a resolution of 1 and then resamples it to the desired interval resolution. I understand it would be more efficient if the user could choose a coarser resolution that also works Usually the largest common divisor between measurement frequency and the interval, which here would be 5.

However, I’m concerned about scenarios where the user specifies a different resolution, such as 3. I was thinking the upsampling process could handle remainders like this:

[
  { "timestamp": "2023-12-12T00:00:00.000Z", "duration": 3, "carbon": 3 },
  { "timestamp": "2023-12-12T00:00:03.000Z", "duration": 3, "carbon": 3 },
  { "timestamp": "2023-12-12T00:00:06.000Z", "duration": 3, "carbon": 3 },
  { "timestamp": "2023-12-12T00:00:09.000Z", "duration": 1, "carbon": 1 }
]

I am struggling to see how resampling to a resolution of 5 could work from here. Is there a flaw in my logic somewhere?

@jmcook1186
Copy link
Contributor Author

hi @mouhamadalmounayar - thanks a lot for looking into this issue - it's a bit of a thorny one.

There will need to be checks in place that the given timestep does not create issues for the resampling.

Maybe we have to assert that the resolution is smaller or equal to the smallest measurement frequency (across all tree components).

The problem with remainders is that it has to be the same across all components of the tree. Seems like this would only occasionally work.

I think it's useful to go through the design process and try to make this work because the pay off is some potentially substantial performance gains for time-sync, but if it turns out to be too complex (i.e. error-prone) we can just keep things the way they are.

@mouhamadalmounayar
Copy link
Contributor

hello again @jmcook1186, I looked a bit more into it, and I believe that the chosen time step should adhere to the following constraints:

  • It must be a divisor of the desired interval.
  • It must be a divisor of all the measurement frequencies (That way we won't have to deal with remainders).
  • It must be a divisor of the gaps and the padding.

Otherwise I don't think resampling would be possible.
Do you agree?

If one of those checks fail, we can always reset the resolution to 1, and maybe throw a warning.

For example, for this observation array :

 {
   "timestamp": "2023-12-12T00:00:00.000Z",
   "duration": 120
 },
  {
   "timestamp": "2023-12-12T00:02:00.000Z",
   "duration": 400
 },

with a desired interval of 10 the user can choose a resolution of 5 or even 10 instead of the predefined 1s resolution.

@jmcook1186
Copy link
Contributor Author

Hi @mouhamadalmounayar, yes I think I agree with those constraints.

@mouhamadalmounayar
Copy link
Contributor

Is it possible to be assigned to work on this ? @jmcook1186

@jmcook1186
Copy link
Contributor Author

Absolutely - it's all yours

@jmcook1186 jmcook1186 moved this from Parked to In Progress in IF Aug 21, 2024
@zanete zanete moved this from In Progress to Pending Review in IF Aug 27, 2024
@zanete zanete linked a pull request Aug 27, 2024 that will close this issue
9 tasks
@narekhovhannisyan
Copy link
Member

@zanete @jmcook1186 @mouhamadalmounayar I have left comments on PR.

@github-project-automation github-project-automation bot moved this from Pending Review to Done in IF Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue This issue is a good one for someone looking to get involved with this project/initiative.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants