WattTime Model Implementation #130
Conversation
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
gnanakeethan
changed the title
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
Signed-off-by: Gnanakeethan Balasubramaniam <[email protected]>
There was a problem hiding this comment.
- Very light on documentation. I've given the same feedback on lack of documentation many times before, it's not going to change. Please can you assume that for every issue ongoing that documentation is going to be a strong requirement. Both the readme and the functions, this one has one on authentication which we've never used before and has some specific functionality with respect to auth and none of it is documented as far as I can see.
- Also the original issue had requirements on how to use the WattTime API so we don't break it's rate limits. We need to think much more carefully about this approach, please re-read the original issue and my comments in this PR.
| name: string | undefined; | ||
| baseUrl = 'https://api2.watttime.org/v2'; | ||
|
|
||
| async authenticate(authParams: object): Promise<void> { |
There was a problem hiding this comment.
It how this auth work documented anywhere @gnanakeethan? Looks like you have to specificy the end var where the username and password is stored, is that right.
Please document this fully both in the comments and in the readme.md.
There was a problem hiding this comment.
added in documentation.
| if (!Array.isArray(observations)) { | ||
| throw new Error('observations should be an array'); | ||
| } | ||
| observations = await Promise.all( |
There was a problem hiding this comment.
@gnanakeethan please not this way, as mentioned in the issue #125
This is the requirement:
"You will need to pass in the start time for the first observation and the start time for the last observation.
It seems to return all the data in 5 min increments, need to figure out how to map this to your series of observations which might not on the same 5 min time series. Investigate and propose an idea to the team for us to discuss before you implement."
The WattTime API has rate limits so we want to call the API as little as possible. From the docs you can call it once with a large time range and it returns an array of results for smaller intervals. So figure out the total time range for the whole series of observations, call the API for that time range and then try to figure out from the responses how to allocate to the individual observations.
The way you are proposing to use here will break their API limits very quickly. Please investigate and propose an alternative route as described in the issue if what I propose doesn't work but the current approach is not approved.
There was a problem hiding this comment.
@jawache that might require extensive processing before/after we obtain the data from Watt-Time API.
I would suggest we create an intermediary mechanism which groups the data to 5 min intervals / larger intervals before we use the watt-time model.
Also we need to consider about the arbitrary durations this model has to execute.
There was a problem hiding this comment.
Please expand on this with:
- An impl with several observations, makes one WattTime API call and each observation has it's own grid-ci.
- What happens when their is an authentication error.
- What happens when the Wattime API returns error codes (like API rate limited?)
- Add documentation so we can understand what your testing without reading the code.
There was a problem hiding this comment.
@jawache I am not testing the response from the API in the unit-test scenario. I am mocking the response.
I will run it via the RIMPL locally to test this.
There was a problem hiding this comment.
added some custom errors to be thrown when there is no data.
| }) | ||
| ); | ||
|
|
||
| return Promise.resolve(observations); |
There was a problem hiding this comment.
| return Promise.resolve(observations); | |
| return observations; |
The function is async so anything returned is treated as a resolve and anything that you throw is treated as a reject.
Types of changes
A description of the changes proposed in the Pull Request
Implementation of WattTime Model for calculating the energy intensity in gCO2e/KWh.