print selene_sdk version, add config and model file to output, add ra… by ygliu2016 · Pull Request #191 · FunctionLab/selene

ygliu2016 · 2022-12-15T03:00:17Z

…ndom suffix to output directory name

Reference Issues/PRs

What does this implement/fix? Explain your changes.

What testing did you do to verify the changes in this PR?

…ndom suffix to output directory name

kathyxchen · 2022-12-20T17:13:33Z

Thanks Aaron for adding me to this PR, and thank you @ygliu2016 for your work on this! I've left some comments--feel free to tag me in a comment once you've updated / tested the changes based on the review, and let me know if you have any questions in the meantime. Overall it looks good! Thanks again :)

ygliu2016 · 2022-12-20T18:23:27Z

Overall it looks good!

Thanks Aaron for adding me to this PR, and thank you @ygliu2016 for your work on this! I've left some comments--feel free to tag me in a comment once you've updated / tested the changes based on the review, and let me know if you have any questions in the meantime. Overall it looks good! Thanks again :)

H, Kathy, thanks for coming back. Would you please tell me where are your comments? In which repository? Thanks.

kathyxchen · 2022-12-20T18:31:23Z

The comments are directly in the changes you made - so if you scroll up above my comment or click 'files changed' tab you should be able to see what I wrote at the corresponding lines of code!

ygliu2016 · 2023-01-10T16:33:00Z

The comments are directly in the changes you made - so if you scroll up above my comment or click 'files changed' tab you should be able to see what I wrote at the corresponding lines of code!

I am so sorry. But I can not see you comments at repository ygliu2016/selene, branch info.

kathyxchen · 2023-01-10T16:51:04Z

Hi @ygliu2016 , were you able to find the comments after you made the test comment? It's at https://github.com/FunctionLab/selene/pull/191/files "Files Changed" tab

ygliu2016 · 2023-01-11T15:07:56Z

No, I cannot. Unfortunately, I can only see my test comment. Can you comment directly in the code?
Thanks.

kathyxchen

Hi @ygliu2016, I've resubmitted the comments, hopefully this time it works!! Thanks for your patience :)

kathyxchen · 2022-12-20T17:04:50Z

 operations = configs["ops"]

+ #print selene_sdk version
+ if "selene_sdk_version" not in configs:


maybe simplify this to version. I think it's also OK to assume that version is never specified in the config and to add it automatically, e.g. moving from selene_sdk import version to the top of the page. I think you can also directly do import selene_sdk and use selene_sdk.__version__ -- not sure which is better practice.

Not sure about name "selene_sdk_version" vs "version". May want to keep the longer version at this time.

OK I thought about it more, and I think it's better to have this be automatically populated? Let's just remove lines 322 and 323 outright.

Please adjust based on comments above, I think it makes sense to just auto-populate since almost no one will try to specify it manually in their config

kathyxchen · 2022-12-20T17:07:54Z

 np.random.seed(seed)
 torch.manual_seed(seed)
 torch.cuda.manual_seed_all(seed)
+ #torch.backends.cudnn.deterministic = True


why are these commented out?

It is your code, which is not included in the main repository of selene. I am not sure if the code is necessary and thus copied here but commented them. It seems to me that with a seed, there is already deterministic in it.

Thanks for the clarification. Please un-comment this as we need it for ensuring all aspects are deterministic.

Same with the commented out line in 365, if that is my code.

can you remove the commented out lines in 364 and 365?

ygliu2016 · 2023-01-11T15:23:12Z

Thank you very much for the comments and in fact, for your patience as well.

kathyxchen

Added last few comments! Let me know when these final changes have been incorporated and tested. :) Thank you

kathyxchen · 2023-01-19T18:07:51Z

 operations = configs["ops"]

+ #print selene_sdk version
+ if "selene_sdk_version" not in configs:


OK I thought about it more, and I think it's better to have this be automatically populated? Let's just remove lines 322 and 323 outright.

kathyxchen · 2023-01-19T18:08:35Z

 np.random.seed(seed)
 torch.manual_seed(seed)
 torch.cuda.manual_seed_all(seed)
+ #torch.backends.cudnn.deterministic = True


Thanks for the clarification. Please un-comment this as we need it for ensuring all aspects are deterministic.

kathyxchen · 2023-01-19T18:08:57Z

 np.random.seed(seed)
 torch.manual_seed(seed)
 torch.cuda.manual_seed_all(seed)
+ #torch.backends.cudnn.deterministic = True


Same with the commented out line in 365, if that is my code.

jzthree · 2023-01-21T01:56:04Z

I think torch.backends.cudnn.deterministic = True is much slower, and that's probably why it was commented out before

kathyxchen · 2023-01-21T01:57:39Z

Oh thanks - that’s good to know! @jzthree

kathyxchen

hi @ygliu2016, I made another pass - some of the conversations you marked resolved I had some minor comments to fully resolve them. Can you tag me when you've finished the changes so I know to review this PR again? thanks!

kathyxchen · 2023-02-21T20:41:22Z

 random_seed: 1447
 output_dir: ./training_outputs
-create_subdirectory: False
+create_subdirectory: True


I assume this was changed because of testing - can this be changed back since it's not relevant to the PR?

kathyxchen · 2023-02-21T20:41:55Z

+ # copy model file or directory to output
+ model_input = configs['model']['path']
+ if os.path.isdir(model_input): # copy the directory
+ shutil.copytree (model_input, 


spacing here has not been fixed still. Please adjust so it adheres to the current style in the codebase, thank you!

kathyxchen · 2023-02-21T20:42:13Z

+ configs = load_path(configs_file, instantiate=False)
 operations = configs["ops"]

+ #print selene_sdk version


remove commented out print statement

kathyxchen · 2023-02-21T20:43:29Z

 operations = configs["ops"]

+ #print selene_sdk version
+ if "selene_sdk_version" not in configs:


Please adjust based on comments above, I think it makes sense to just auto-populate since almost no one will try to specify it manually in their config

kathyxchen · 2023-02-21T20:48:48Z

 np.random.seed(seed)
 torch.manual_seed(seed)
 torch.cuda.manual_seed_all(seed)
+ #torch.backends.cudnn.deterministic = True


can you remove the commented out lines in 364 and 365?

print selene_sdk version, add config and model file to output, add ra…
d939968
…ndom suffix to output directory name

aaronkw requested a review from kathyxchen December 20, 2022 14:45

ygliu2016 commented Jan 10, 2023
View reviewed changes

Comment threadselene_sdk/utils/config_utils.py

ygliu2016 closed this Jan 11, 2023

ygliu2016 reopened this Jan 11, 2023

kathyxchen reviewed Jan 11, 2023
View reviewed changes

Address review comment of Kathy
df6e53a

kathyxchen requested changes Jan 19, 2023
View reviewed changes

aaronkw requested a review from kathyxchen February 21, 2023 16:49

kathyxchen requested changes Feb 21, 2023
View reviewed changes

Uh oh!

Conversation

ygliu2016 commented Dec 15, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

What testing did you do to verify the changes in this PR?

Uh oh!

kathyxchen commented Dec 20, 2022

Uh oh!

ygliu2016 commented Dec 20, 2022

Uh oh!

kathyxchen commented Dec 20, 2022

Uh oh!

ygliu2016 commented Jan 10, 2023

Uh oh!

Uh oh!

kathyxchen commented Jan 10, 2023

Uh oh!

ygliu2016 commented Jan 11, 2023

Uh oh!

kathyxchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ygliu2016 commented Jan 11, 2023

Uh oh!

kathyxchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jzthree commented Jan 21, 2023

Uh oh!

kathyxchen commented Jan 21, 2023 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kathyxchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kathyxchen commented Jan 21, 2023 via email •
edited
Loading