Added boolean disables, generation of train (metrics.csv) and test metrics (test_metrics.csv) #397
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
|
|
📌 Cov diff with main: No success to gather report. 😿 📌 Overall coverage: No success to gather report. 😿 |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
|
||
|
|
||
| class ArimaOperatorModel(ForecastOperatorBaseModel): | ||
| """Class representing ARIMA operator model.""" | ||
|
|
||
| def __init__(self, config: ForecastOperatorConfig): | ||
| super().__init__(config) | ||
| self.train_metrics = False |
There was a problem hiding this comment.
If the all models require these extra attributes, maybe we can add them to the base model class?
There was a problem hiding this comment.
These values were defined for each model inside _generate_report method. I have abstracted them out as attributes because I needed them outside of that method.
The train_metrics value is True/False according to the model so I am not sure if we can define them in base_model. cc: @ahosler
There was a problem hiding this comment.
One thing you could do is add self.train_metrics = False to the base class, and then update it to True only in prophet and neuralprophet so that if we add model classes in the future train_metrics is disabled by default.
| self.preprocessing = ( | ||
| self.preprocessing if self.preprocessing is not None else True | ||
| ) | ||
| self.explain = self.explain if self.explain is not None else False | ||
| self.generate_report = ( |
There was a problem hiding this comment.
Not sure, that I understand this logic.
self.generate_report if self.generate_report is not None else True
Do we want to make True as default option for case when user didn't specify the value?
Could you add please a short comment there in this case.
There was a problem hiding this comment.
Yes we want the default to be True when user doesn't specify anything. I have added the comments.
| @@ -168,7 +168,7 @@ spec: | |||
| type: dict | |||
| type: dict | |||
|
|
|||
| report_file_name: | |||
| report_filename: | |||
There was a problem hiding this comment.
We need to be cautious with making such changes, if somebody uses old config, then they would start getting some errors. However the change looks good to me :)
cc: @ahosler
There was a problem hiding this comment.
Yes! Varsha is using this ticket to go through and clean up all the names for GA. After we merge with main, we will not be changing without backward compatibility. Any other inconsistencies you notice please flag now! Thanks
| self.generate_report = ( | ||
| self.generate_report if self.generate_report is not None else True | ||
| ) | ||
| self.generate_metrics = ( |
There was a problem hiding this comment.
The both generate_report and generate_metrics need to be added to a schema?
There was a problem hiding this comment.
Yeah, they should be added. I made the changes.
| type: string | ||
| default: forecast.csv | ||
| meta: | ||
| description: "Placed into output_directory location. Defaults to forecast.csv" |
There was a problem hiding this comment.
This is a great idea! Thank you!
|
|
||
|
|
||
| class ArimaOperatorModel(ForecastOperatorBaseModel): | ||
| """Class representing ARIMA operator model.""" | ||
|
|
||
| def __init__(self, config: ForecastOperatorConfig): | ||
| super().__init__(config) | ||
| self.train_metrics = False |
There was a problem hiding this comment.
One thing you could do is add self.train_metrics = False to the base class, and then update it to True only in prophet and neuralprophet so that if we add model classes in the future train_metrics is disabled by default.
|
📌 Cov diff with main: 📌 Overall coverage: No success to gather report. 😿 |
|
📌 Cov diff with main: No success to gather report. 😿 📌 Overall coverage: No success to gather report. 😿 |
|
📌 Cov diff with main: 📌 Overall coverage: |
…s-as-csvs' of https://github.com/oracle/accelerated-data-science into feature/add-boolean-disables-and-save-train-test-metrics-as-csvs
…and-save-train-test-metrics-as-csvs
|
📌 Cov diff with main: 📌 Overall coverage: |
ahosler
deleted the
feature/add-boolean-disables-and-save-train-test-metrics-as-csvs
branch
https://jira.oci.oraclecorp.com/browse/ODSC-48872
https://jira.oci.oraclecorp.com/browse/ODSC-48205
https://jira.oci.oraclecorp.com/browse/ODSC-48415