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

Dev ticket - remove historical arg from MockObservations plugin Generators #24

Open
pazbardanl opened this issue Jan 30, 2024 · 4 comments
Assignees
Labels
good first issue Good for newcomers help-wanted suggested for community contributions tests related to unit or integration tests

Comments

@pazbardanl
Copy link
Contributor

Refactor MockObservation plugin and its generators so that they don't rely on historical data

Story

The benefit of this refactor is ease of development of mock observations generators:
As a MockObservation plugin developer, I'd like to be able to develop a mock observations generator without the burdensome logic of dealing with historical data. Instead, I want to write a generator code that generates a set of mock observations in a single invocation.

Rationale

Originally, the classes that implement Generator interface were meant to be invoked once per observation. As such, if the logic for generating the new observation depends on previous generated values, the generator must accept this historical data as input.
This makes the overall logic of such generators more complex, and may lead to lesser performance.
_Example: imagine a sine-like generator of a certain metric. To produce the next data point, the generator needs to analyze the historical data to understand where does the next data point fit in a single period of the sine signal. Instead, we would rather such a generator to operate only based on period and amplitude of the sine. _

Implementation details

  1. Refactor Generator interface:
    next(historical: Object[] | undefined): Object; --> generate(): Object[];
    i.e a generator class will not generate a single object (observation) per invocation but rather an array of objects (observations) per invocation.
  2. Refactor change MockObservation plugin's index.ts accordingly: execute(...) method should invoke every generator once and then 'stich' the results together into the returned outputs.
  3. Align all existing generators' unit tests with this change.
  4. MockObservations unit test should not be impacted by this change.

Priority

3/5
mainly due to expected usage of MockObservations in CarbonHack2024 and the expectation that some teams would want to come up with their custom generators

Scope

MockObservations plugin itself
Its Generator interface and existing generators (CommonGenerator and RandIntGenerator at the moment)

Size

Estimated <=0.5d for 1 dev

What does "done" look like?

MockObservations plugin, Generator interface and all existing generators are refactored
Impact-engine execution is successful with the same test impl (if: examples/impl/test/mock-observations.yml)

Does this require updaes to documentation or other materials??

MockObservations plugin README file is to be updated according to this change.

What testing is required?

Plugin and generators unit tests are updated and passing
Impact-engine execution is successful with the same test impl (if: examples/impl/test/mock-observations.yml)

Is this a known/expected update?

Discussed at IF weekly Jan-18 2024 when MockObservations plugin was demo'ed.

@pazbardanl pazbardanl added good first issue Good for newcomers tests related to unit or integration tests model labels Jan 30, 2024
@pazbardanl pazbardanl self-assigned this Jan 30, 2024
@jmcook1186
Copy link
Contributor

Hi @pazbardanl are you actively working on this or is this open for contributions?

@pazbardanl
Copy link
Contributor Author

Open for contributions. I'm here for any questions or guidance needed.
If this is not picked up by next sprint I might take it, want to be done with the visualization stuff before I move on to something else

@jmcook1186 jmcook1186 removed the model label Mar 20, 2024
@jmcook1186 jmcook1186 added the help-wanted suggested for community contributions label Apr 10, 2024
@jmcook1186
Copy link
Contributor

@pazbardanl does this ticket still make sense to keep open?

@pazbardanl
Copy link
Contributor Author

This refactor still makes sense. Not sure about priority though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help-wanted suggested for community contributions tests related to unit or integration tests
Projects
Status: Backlog
Development

No branches or pull requests

2 participants