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 support for EIA forecasts endpoint #247
Conversation
vortexasdk/api/eia_forecast.py
Outdated
|
||
|
||
@dataclass(frozen=True) | ||
class EIAForecast(IDName, FromDictMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This EIAForecast
class currently inherits from IDName - which should only be the case if each EIAForecast record returned by the API has both an id
attribute and a name
attribute (https://github.com/VorTECHsa/python-sdk/blob/master/vortexasdk/api/shared_types.py#L41). @dufia This is likely the cause of the failing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved 👍 thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also want to update the about-endpoints page that lists the available endpoints.
just notices the cargo time series endpoint is missing - while we're here can we add that?
http://localhost:8000/endpoints/about-endpoints/
https://github.com/VorTECHsa/python-sdk/blob/master/docs/endpoints/about-endpoints.md
def search( | ||
self, | ||
preset: str = None, | ||
filter_time_min: str = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type here should be datetime, rather than string. We'll also want sensible default datetimes rather than the bool False
- Vortexa's data science based forecast of the EIA number to be published on the week | ||
- Actual EIA import/export numbers as published by the EIA Weekly Supply Estimates report | ||
- EIA stocks (kbl) | ||
- Cover (days of Supply for the whole of the US, as published by the EIA Weekly Supply Estimates report) | ||
- Refinery runs (refiner “Percent Operable Utilization” as published by the EIA Weekly Supply Estimates report) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - should we make this even clearer by specifying the column name first, then the accompanying documentation? something like:
runs: Refinery runs (refiner...
this means it'll be easier for clients to lookup the functionality associated with a col name
Represent EIA forecasts as a `pd.DataFrame`. | ||
|
||
# Arguments | ||
columns: The EIA forecasts features we want in the dataframe. Enter `columns='all'` to include all features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columns: The EIA forecasts features we want in the dataframe. Enter `columns='all'` to include all features. | |
columns: The EIA forecasts columns we want in the dataframe. Enter `columns='all'` to include all columns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the world of datascience, the term features normally refers to the input features of a model. In this sense of the word, we're better off using 'column' rather than features, as these aren't input features.
filter_time_max: str = False, | ||
) -> EIAForecastResult: | ||
""" | ||
Find EIA forecasts for a given date preset and date range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we mean date preset here? It's more of a geography & product preset right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, it should've been: "Find EIA forecasts for a given preset and date range."
| 2020-01-31T00:00:00.000Z | 454.96048964485 | 323 | 9541 | 26.5 | 65.9 | | ||
| 2020-01-24T00:00:00.000Z | 545.453497230504 | 579 | 10461 | 25.9 | 61.5 | | ||
| 2020-01-17T00:00:00.000Z | 510.289752707662 | 549 | 10325 | 25.2 | 64.7 | | ||
| 2020-01-10T00:00:00.000Z | 469.841470826967 | | | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we give an accompanying note as to why some of the values are null? Clients will definitely ask this Q - we may as well answer it here once.
Find EIA forecasts for a given date preset and date range. | ||
|
||
# Arguments | ||
preset: the EIA forecasts preset to be returned. Preset can be: 'padd1-gasoline-imports', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explain what a preset is?
Adds the EIA forecast endpoint.