-
Notifications
You must be signed in to change notification settings - Fork 354
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
feat: Added tabular forecasting samples #128
feat: Added tabular forecasting samples #128
Conversation
Here is the summary of changes. You added 1 region tag.
This comment is generated by snippet-bot.
|
|
||
training_task_inputs_dict = { | ||
# required inputs | ||
"targetColumn": target_column, |
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.
Curious as to if we should show comments for each of these values in the sample.
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.
@ivanmkc do you mean comments like # display_name: YOUR_DISPLAY_NAME?
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.
Thanks for reviewing, I was wondering if we should add comments for each param in the samples.
Seems like Yuhan is suggesting to just tell them to read the docstrings.
samples/snippets/create_training_pipeline_tabular_forecasting_sample_test.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,37 @@ | |||
# Copyright 2020 Google LLC |
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.
Same as corresponding classification sample.
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.
I think we don’t really need to have sample variants for get and list methods. I see that we have them for some other kinds of AutoML services and perhaps those need to be removed (in a separate PR).
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.
Sure!
@@ -0,0 +1,44 @@ | |||
# Copyright 2020 Google LLC |
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.
Same as corresponding classification sample.
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.
Is it possible that all the predict tabular samples would be identical? Is so we should consolidate those too perhaps. On the other hand if the sample tests would be very different (Eg you might be looking for different specific fields in the predictions), then let’s keep this.
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.
Predict cannot be used with 'tabular'. They only support batch_predict
, I will think about consolidation when doing batch_predict next week.
a74d6c3
to
e75b880
Compare
…t, fixed get_model_evaluation_tabular_forecasting_sample, and fixed create_training_pipeline_tabular_forecasting_sample
|
||
import predict_tabular_classification_sample | ||
|
||
ENDPOINT_ID = "TODO" |
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.
WIP
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.
Blocked by: https://b.corp.google.com/issues/175666946
Fixing CI errors. |
"transformations": transformations, | ||
"period": period, | ||
|
||
# Objective function the model is to be optimized towards. |
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 it's better to point to the doc pages instead of including the information below here.
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 you mean include a link in the comments?
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.
right - if the information here is available on a particular documentation page.
.sample_configs/variants.yaml
Outdated
@@ -175,7 +177,7 @@ list_hyperparameter_tuning_jobs: | |||
list_model_evaluation_slices: | |||
- '' | |||
list_model_evaluations: | |||
- '' | |||
- tabular_forecasting |
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 would replace the list_model_evaluation sample. Please keep the "default" variant keyed with the empty string.
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.
Also - perhaps we don't need a special list_model_evaluations
variant unless the call to list_model_evaluations
is different in the tabular_forecasting
use case.
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.
Oh I see. Good catch.
@@ -182,6 +183,7 @@ get_model_evaluation_sample: | |||
- model_explanation | |||
get_model_evaluation_slice_sample: {} | |||
get_model_evaluation_tabular_classification_sample: {} | |||
get_model_evaluation_tabular_forecasting_sample: {} |
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.
If the tabular_forcasting
variant for get_model_evaluation
ends up identical with any get_model_evaluation
, please skip it. (and we should separately clean up the others - but perhaps i missed something here)
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.
I got it, we want to consolidate if possible. Let me do another pass.
9f113a4
to
087ff39
Compare
@dizcology can I get another look? |
Added samples and tests for AutoML forecasting