Add new Path solution to ESCWA model #23
Conversation
|
This is great. Just some little ideas:
|
Thanks to @xiaolong0728 for pointing out
|
@xiaolong0728 Not sure about 1, but imo doesn't hurt to keep it in, especially since we don't push them to git. |
|
I think Simon and Noorain should be notified since this change also affects their scripts. Here I only see myself as the reviewer. |
|
FYI @Polichinel and @noorains |
There was a problem hiding this comment.
In short, you should just merge from main as soon as I have merged the last purple_alien. Which, if no new bugs show up, will be Thursday the 13th early in the morning.
This will allow you to either get the model_path from a dedicated function or go directly to the setup_artifacts_path.
Once that is fixed we'll merge
common_utils/set_path.py
Outdated
| PATH_GENERATED = PATH_DATA / "generated" | ||
|
|
||
| return PATH_RAW, PATH_PROCCEDS, PATH_GENERATED | ||
| return PATH_MODEL, PATH_RAW, PATH_PROCESSED, PATH_GENERATED |
There was a problem hiding this comment.
I have kept this as purely a data path thing. The PATH_MODEL can be defined using setup_model_paths().
But I think the way it is used below you might be better off just using setup_artifacts_path()
There was a problem hiding this comment.
So, I'll push the latest changes to main tomorrow. Just running sweep to see that everything is as it should be after incorporating your last comments. After that you should merge from main to this
| @@ -34,7 +37,10 @@ def evaluate_model(model_config): | |||
| """ | |||
| print("Evaluating...") | |||
|
|
|||
| df_calib = pd.read_parquet(model_path/"data"/"generated"/"calibration_predictions.parquet") | |||
| PATH_MODEL, PATH_RAW, PATH_PROCESSED, PATH_GENERATED = setup_data_paths(PATH) | |||
There was a problem hiding this comment.
with the new implementation, you should not get PATH_MODEL from here. You can get it from setup_model_path but given the use below I think you should just use setup_artifacts_path
| @@ -61,7 +67,7 @@ def evaluate_model(model_config): | |||
| [row[col] for col in pred_cols]), axis=1) | |||
| mean_brier_score = df_calib["brier_score"].mean() | |||
|
|
|||
| metrics_dict_path = model_path / "artifacts" / "evaluation_metrics.py" | |||
| metrics_dict_path = PATH_MODEL / "artifacts" / "evaluation_metrics.py" | |||
There was a problem hiding this comment.
then you can also simplify this a bit
| PATH_GENERATED = PATH_DATA / "generated" | ||
|
|
||
| return PATH_RAW, PATH_PROCCEDS, PATH_GENERATED # added in accordance with Sara's escwa branch | ||
| return PATH_RAW, PATH_PROCESSED, PATH_GENERATED # added in accordance with Sara's escwa branch |
There was a problem hiding this comment.
fixed a typo FYI @Polichinel
Adapted src of model training, forecasting and evaluation for the new machine-agnostic path solution using
common_utils/set_pathsIMPORTANT:
I configured
common_utils/set_paths:@xiaolong0728 In your generate_forecast.py script you'll have to change
PATH_RAW, _, PATH_GENERATED = setup_data_paths(PATH)toPATH_MODEL, PATH_RAW, PATH_PROCESSED, PATH_GENERATED = setup_data_paths(PATH)since I added PATH_MODEL to the output of the functionIf you agree with this, please merge the PR.
I'll be tackling refactoring the data partitions next.