Skip to content

Conversation

@brandon-edwards
Copy link
Collaborator

No description provided.

@brandon-edwards brandon-edwards requested a review from psfoley June 29, 2021 00:40
@brandon-edwards
Copy link
Collaborator Author

I have some more testing to do, will re-open when done.

Edwards added 3 commits June 29, 2021 13:52
@brandon-edwards
Copy link
Collaborator Author

Ok re-opening, with the reminder that this is dependent on an openfl pull request(#114)

sarthakpati
sarthakpati previously approved these changes Jun 30, 2021
Copy link
Member

@sarthakpati sarthakpati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sarthakpati
Copy link
Member

I think this is good to merge, what do you think, @psfoley?

"data_path = '/raid/edwardsb/validation_set'\n",
"\n",
"# you can keep these the same if you wish\n",
"best_model_path = '/home/edwardsb/.local/workspace/checkpoint/' + checkpoint_folder + '/best_model.pkl'\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these paths, it might be worth doing something like this so the user doesn't need to modify their home directory:

from pathlib import Path
home = str(Path.home())
best_model_path = f'{home}/.local/workspace/checkpoint/{checkpoint_folder}/best_model.pkl'
outputs_path = f'{home}/.local/workspace/checkpoint/{checkpoint_folder}/model_outputs'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch Patrick, thanks. I missed that this was in the part I was suggesting did not need to be changed as well.

"# you will need to specify the correct experiment folder and the parent directory for\n",
"# the data you want to run inference over\n",
"checkpoint_folder='experiment_1'\n",
"data_path = '/raid/edwardsb/validation_set'\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this clear that this is the validation data downloaded from the challenge? i.e.

data_path = /PATH/TO/CHALLENGE_VALIDATION_DATA

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@psfoley
Copy link
Collaborator

psfoley commented Jul 1, 2021

I just have two small comments related to replacing hard coded paths. Beyond that, this is ready to merge 👍

Copy link
Collaborator Author

@brandon-edwards brandon-edwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the changes Patrick suggested and did another run test.

Copy link
Collaborator

@psfoley psfoley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@psfoley psfoley merged commit 3fd2bfb into FeTS-AI:main Jul 1, 2021
@brandon-edwards brandon-edwards deleted the validation_outputs branch July 1, 2021 22:45
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.

3 participants