Skip to content
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

Combine figures 3 and 4 #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RobvanGastel
Copy link

@RobvanGastel RobvanGastel commented May 19, 2023

I combined experiments 3 and 4 for issue #19.

Only 2 things remain unsolved. First, on line 84 the n_params is hardcoded to 3, should this be passed like in Figure 3?

size=(cfg.environment.n_parallel_experiments, 3)

Second, experiment 4 is not averaged on the experiment side, should we average on the experiment side or use weights and biases?

If these are resolved we can remove Figure_3 and Figure_4.

Changes

  • Added a .gitignore file to ignore pycache directories
  • Added argparse arguments to select config files

@RobvanGastel
Copy link
Author

Set line 84 to n_params instead of hardcoding, and averaging using wandb grouping for figure 4, the ticket should be complete now.

@Johnny1188
Copy link
Collaborator

Good job! Just a few points:

  • It seems like the merged script doesn't contain the latest version of the original two scripts (for example loading from a checkpoint is missing). There might have been some PRs on the master branch that your fork didn't mirror, could you please update it?
  • Having multiple trials right there in the script is a good idea but I think the metrics and model checkpoints that get saved locally overwrite the metrics and checkpoints from the earlier trials (the save path cfg.save_path is still the same).

@RobvanGastel
Copy link
Author

Good catch on the save_path for the trials, I will add the trial id to separate every output and also merge with the checkpoint on the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants