-
Notifications
You must be signed in to change notification settings - Fork 24
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
Plot training calibration, PR, and ROC curves; logging of label breakdown and number of epochs #332 #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments mostly about the handling of the generator workers, we should be consistent and handle both the training and validation generators. also let's get review of @lucidtronix or @ndiamant
ml4cvd/models.py
Outdated
@@ -1016,6 +1016,7 @@ def train_model_from_generators( | |||
inspect_show_labels: bool, | |||
return_history: bool = False, | |||
plot: bool = True, | |||
plot_train_curves: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding the argument here as plot_train_curves
implies this function will do the plotting or have some functionality relating to it. instead it's just deferring the worker management to the caller of the function. maybe we can rename this argument for train_model_generators
to defer_worker_halt
or something similar?
ml4cvd/recipes.py
Outdated
out_path = os.path.join(args.output_folder, args.id + '/') | ||
test_data, test_labels, test_paths = big_batch_from_minibatch_generator(generate_test, args.test_steps) | ||
train_data, train_labels = big_batch_from_minibatch_generator(generate_train, args.training_steps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the size of the big_batch returned here? training_steps is usually alot larger than test_steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
train big_batch shape = (25600, 2500, 12), as opposed to (2048, 2500, 12) for test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a need to return train_paths
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to return it since it isn't used in plotting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is optional, if provided it will be used to label outliers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, please remove the 2 extra files though and also request review from a Broadie!
._.DS_Store
ml4cvd/._models.py
This repo has a Also, ensure |
those aren't currently in the codebase https://github.com/broadinstitute/ml/blob/106a1cca25a1e2f68cc4db99658ff38d4f5a94ce/.gitignore I wonder what's generating the |
It seems like they're metadata files created by Mac that get separated out when I push (can't see them on my computer but I see them on the github website)? |
would recommend doing before add and commit to git, can also do we could also just add |
Closing this issue for now. I've merged the changes into a separate fork ( |
resolves #332
resolves #216
Added argument plot_train_curves (defaults to False) to plot calibration, PR, and ROC curves for training set.
Reports the label breakdown for train/valid/test at the end of train mode
Reports the number of epochs completed