-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Tensorboard path generalisation #804
Tensorboard path generalisation #804
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.
Looks nice, Thank you for youo help.
Could you pls adress some fomatting issues...
Allow experiment names to be empty, in which case no per-experiment subdirectory will be created and checkpoints will be saved in the directory given by the save_dir parameter.
FYI, I slightly simplified my test. It still tests as much as the existing ones but doesn't check for the tensorboard output directory being created. Perhaps tensorboard doesn't create it immediately when saving in earlier versions. I couldn't get it to fail locally and it works for non-minimal versions. |
version (int): Experiment version. If version is not specified the logger inspects the save | ||
name (str): Experiment name. Defaults to "default". If it is the empty string then no per-experiment | ||
subdirectory is used. | ||
version (int|str): Experiment version. If version is not specified the logger inspects the save | ||
directory for existing versions, then automatically assigns the next available version. |
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.
this should have extra tab spacing as it belongs as a block to an argument
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.
Done
@property | ||
def root_dir(self): | ||
""" Parent directory for all tensorboard checkpoint subdirectories. | ||
If the experiment name parameter is None or the empty string, no experiment subdirectory is used |
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.
there is no for extra tab spacing
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 think it now follows the format used elsewhere. We're getting there.
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.
the point/issue is that still do not have unified formatting everywhere yet...
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.
LGTM
Before submitting
What does this PR do?
Fixes issue 793
You can (say) specify the version on the command line or generate it from the current datetime, which I find more meaningful than "version_73".
Likewise if the tensorboard data is not going to be preserved, it's much more readable in the tensorboard gui to skip the experiment name.
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃
I learnt some things