Conversation
| path = get_path_model(run_id=run_id) | ||
| else: | ||
| path = Path(model_path) / run_id | ||
| path = _get_shared_wg_path() / "models" / run_id |
There was a problem hiding this comment.
There is already a method for this.
I think we shouldn't change this. It is used when an explicit model_path was given. Otherwise the if-else block is redundant and we can just use the part in the if-block.
There was a problem hiding this comment.
Indeed the else is redundant, I will revert and add a logging instead
| """ | ||
|
|
||
| path_run = Path(cf.model_path) / run_id | ||
| path_run = _get_shared_wg_path() / "models" / run_id |
There was a problem hiding this comment.
Same applies here. We could use this method directly. Maybe we can add the same if-else block as in the previous case. This enables the user to not add a model_path to the config and just use the regular path. But I'm not entirely sure if this is possible here. Maybe someone with more experience on the model and training process knows better. @sophie-xhonneux ?
|
Thank you @TillHae for taking the time to review this PR, I added the requested changes ^^ |
2 participants
Description
Issue Number
Closes #1790
Is this PR a draft? Mark it as draft.
Checklist before asking for review
./scripts/actions.sh lint./scripts/actions.sh unit-test./scripts/actions.sh integration-testlaunch-slurm.py --time 60