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

Fix logger, tensorboard #610

Merged
merged 6 commits into from
Dec 8, 2019
Merged

Fix logger, tensorboard #610

merged 6 commits into from
Dec 8, 2019

Conversation

Borda
Copy link
Member

@Borda Borda commented Dec 8, 2019

What does this PR do?

Fixes #607 to be able to do #609

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.

Comment on lines 67 to 73
try:
# in case converting from namespace, todo: rather test if it is namespace
params = vars(params)
except TypeError:
pass
if params is not None:
self.experiment.add_hparams(hparam_dict=dict(params))
Copy link
Contributor

Choose a reason for hiding this comment

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

What problem is this fixing? The current implementation is consistent with all the other loggers.

Copy link
Member Author

Choose a reason for hiding this comment

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

a dictionary cannot be converted via vars

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for future reference, do we pass a dict anywhere? I thought we always passed some form of Namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

In almost all tests... =)

@neggert
Copy link
Contributor

neggert commented Dec 8, 2019

Thanks for the bug fixes. I think maybe I did a rebase right at the end and forgot to re-run the tests. I promise they were all passing at some point.

@Borda
Copy link
Member Author

Borda commented Dec 8, 2019

well, not only, also flush is missing in older PT... :]

@Borda Borda changed the title Fix logger [wip] Fix logger, tensorboard Dec 8, 2019
@williamFalcon williamFalcon merged commit 5d00e62 into Lightning-AI:master Dec 8, 2019
@Borda Borda deleted the fix-logger branch December 8, 2019 16:04
@neggert
Copy link
Contributor

neggert commented Dec 8, 2019

Yeah, missing the flush on Torch 1.1 was my mistake.

@Borda
Copy link
Member Author

Borda commented Dec 8, 2019

It is fixed, but more serious is missing add_hparams in 1.1 which means that they are not exported at all...

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