Hyperparameter tuning#69
Conversation
|
@meiertgrootes and @rogerkuou this PR adds support for hyper parameter tuning using ray. I added an example notebook that can be run locally using cpu. In meantime, I'm testing the workflow in levante. See #40 for details on tuning strategy. |
|
Hi @SarahAlidoost for now I will only review and test this on my local. Will wait for your signal about the HPC test |
rogerkuou
left a comment
rogerkuou
left a comment
There was a problem hiding this comment.
Hi @SarahAlidoost , thanks! Just some small comments. After fixing them please feel free to merge!
There was a problem hiding this comment.
Comment for the last cell, the experiment_path changes per run. I propose it can be changed to:
check_best_model(Path(best_result.path).parent, test_dataset, run_dir)so two changes:
- I think
check_best_modelcan acceptpathlib.Pathso no need to convert to str? - Use
Path(best_result.path).parenttorefer the experiment path
There was a problem hiding this comment.
Comment for the last cell, the
experiment_pathchanges per run. I propose it can be changed to:check_best_model(Path(best_result.path).parent, test_dataset, run_dir)so two changes:
- I think
check_best_modelcan acceptpathlib.Pathso no need to convert to str?
right! I checked it, it accepts both path and str. it is fixed now.
- Use
Path(best_result.path).parenttorefer the experiment path
This is one way of getting the experiment path. If the best_result is not available in the current run, the path should be defined by user. I added some comments to the notebook.
There was a problem hiding this comment.
thanks @SarahAlidoost ! Just one more thing, in your current version, I think results.experiment_path points to one level deeper: ......./runs_daily/_train_2026-07-16_16-48-59/_train_7a5a1_00001_1_patch_size=4_2026-07-16_16-48-59, instead of ......./runs_daily/_train_2026-07-16_16-48-59/. Hence I added the .parent`, maybe we should still keep it?
I agree the comment is already quite useful though.
There was a problem hiding this comment.
in my version results.experiment_path points to first level e.g. ./runs_daily/_train_2026-07-16_16-48-59/. Can you check your version again? Note in the last commit I removed the function check_best_model and moved everything to the notebook.
@rogerkuou Hi, thanks for the review. I won't merge this PR before getting results of the notebook on levante, checking ray and gpus. |
| plt.show() | ||
|
|
||
|
|
||
| def data_split( |
There was a problem hiding this comment.
Two (semi-)minor points here:
-
this assumes that train/test/validation will cover contiguous ranges in time. That is a choice we are making, but may be sth that would benefit from being more flexible. For the time being I'd say leave it as it is, in order to proceed, but maybe worth returning to as a separate issue
-
to nit pick a little bit: technically hyper-parameter tuning, which is performed on the basis of the test score results requires an additional hold-out set to finally asses model performance on pristine data. Since we are only extending to 2022 we could use 2023, but this does open the issue of actual physical temporal drift.
There was a problem hiding this comment.
- agree, I've explained the choice here. In the MAESSTRO code, they did similar, different years for training and validation (like 2012 vs 2011). I'll submit a new issue to consider random split in time, which needs trying out different strategies again.
- Right, we have three datasets: train/validation/test. In our training workflow, we use validation set and hold out the test data. The model haven't seen the test data.
| num_samples=tune_config["num_trials"], | ||
| max_concurrent_trials=tune_config["max_concurrent_trials"], | ||
| ), | ||
| param_space=tune_config, |
There was a problem hiding this comment.
this will feed all entries in tune_config in as parameters in the Hyperparameter optimization. This will (likely) result in things like num_trials, cpu+per_trial etc. being tracked in logs and dashboards as constant parameters, impacting overview. Could we consider cleaning out non-optimisation parameters, before injection?
There was a problem hiding this comment.
good point! In their example, both static arguments and hyper parameters are in one config dictionary passed to the training function. But as you mentioned, it is cleaner to split them. I found that is possible to pass arguments in ray.tune.with_parameters and fixed this.
meiertgrootes
left a comment
meiertgrootes
left a comment
There was a problem hiding this comment.
Some minor comments, but nothings to stop merging.
@meiertgrootes thanks for the review. I addressed your comments. I'm still testing this PR on HPC using cuda. Once passed, I'll merge this PR. |
3 participants
closes #40