-
Notifications
You must be signed in to change notification settings - Fork 50
feat: add DataFrame.ai.forecast() support #1828
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
Conversation
bigframes/operations/ai.py
Outdated
|
||
self._df: bigframes.dataframe.DataFrame = df | ||
self._base_bqml = ml_core.BaseBqml(session=df._session) |
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.
Can you inject BaseBqml as a constructor parameter (default to None)? It might make testing easier if we want to fake/mock BaseBqml in the future.
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.
done.
"confidence_level": confidence_level, | ||
} | ||
if id_columns: | ||
options["id_cols"] = id_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.
Maybe we should verify the validity of timestamp_column
and data_column
, and raise error if necessary?
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.
You mean the data types? We rely on the checks from the backend. Basically client libraries shouldn't do too much checks unless those are only specific to client.
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.
No I meant the existence of these columns. If the user has made some typos, we want the code to fail fast. The other methods have this check too.
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.
done.
bigframes/operations/ai.py
Outdated
|
||
self._df: bigframes.dataframe.DataFrame = df | ||
self._base_bqml: ml_core.BaseBqml = base_bqml |
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.
Let's also provide a default BaseBqml instance if None
is provided: It should be just ml_core.BaseBqml(df._session)
.
By doing this, we don't need to provide one in that dataframe ai property.
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.
done.
Thanks! |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕