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.

Sign up for GitHub

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

Jump to bottom

Bug fix hparam logging with metrics #1647

Merged
merged 4 commits into from
May 12, 2020
Merged

Bug fix hparam logging with metrics #1647

merged 4 commits into from
May 12, 2020

Conversation

justusschock
Copy link
Member

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1228 and follows #1630

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 🙃

@justusschock justusschock changed the title Buf fix hparam logging with metrics Bug fix hparam logging with metrics Apr 28, 2020
@mergify mergify bot requested a review from a team April 28, 2020 07:18
@justusschock justusschock mentioned this pull request Apr 28, 2020
Closed
@festeh
Copy link
Contributor

festeh commented Apr 29, 2020

I remember I did exactly inverse PR in #777. The problem was that for each single run it created two records in the Tensorboard (because new SummaryWriter was created for hparams) and runs view was really cluttered. I wonder if this PR will reintroduce this behavoir.

@Borda Borda added the bug Something isn't working label Apr 29, 2020
@justusschock
Copy link
Member Author

Okay, I think I already know a solution to this one :) Thanks for the hint @festeh

olineumann
olineumann suggested changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@olineumann olineumann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

'hparams' import can be deleted and tests failing because of None dicts.

pytorch_lightning/loggers/tensorboard.py Show resolved Hide resolved
@Borda Borda added this to the 0.7.6 milestone Apr 30, 2020
@Borda Borda added the waiting on author Waiting on user action, correction, or update label Apr 30, 2020
@awaelchli awaelchli mentioned this pull request May 4, 2020
Closed
@Borda
Copy link
Member

Borda commented May 10, 2020

@justusschock how about the suggestions from @olineumann?

@pep8speaks
Copy link

pep8speaks commented May 12, 2020 •
edited
Loading

Hello @justusschock! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-12 09:45:06 UTC

@justusschock
Copy link
Member Author

Reverted some of the changes and added logging for metrics if provided

@justusschock justusschock marked this pull request as ready for review May 12, 2020 07:31
@justusschock justusschock mentioned this pull request May 12, 2020
Closed
@justusschock
Copy link
Member Author

Only the formatting test fails and this one is not related to this PR. I think this should be ready to merge @Borda @williamFalcon

@justusschock justusschock requested review from Borda, awaelchli, williamFalcon and a team and removed request for a team May 12, 2020 07:49
@Borda
Copy link
Member

Borda commented May 12, 2020

I have been fixing formatting in #1786 it is related to flake8 check all errors now

@Borda Borda changed the title Bug fix hparam logging with metrics [blocked by #1786] Bug fix hparam logging with metrics May 12, 2020
@mergify mergify bot requested a review from a team May 12, 2020 08:36
@Borda Borda added the ready PRs ready to be merged label May 12, 2020
@Borda Borda force-pushed the hparams_logger branch from bc9ecd5 to 7206828 Compare May 12, 2020 09:45
@Borda Borda changed the title [blocked by #1786] Bug fix hparam logging with metrics Bug fix hparam logging with metrics May 12, 2020
@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #1647 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1647   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          69      69           
  Lines        4316    4318    +2     
======================================
+ Hits         3805    3807    +2     
  Misses        511     511

@williamFalcon williamFalcon merged commit 5f29239 into master May 12, 2020
@Borda Borda deleted the hparams_logger branch May 12, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olineumann olineumann olineumann approved these changes

@Borda Borda Borda approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working ready PRs ready to be merged waiting on author Waiting on user action, correction, or update
Projects
None yet
Milestone
pl:0.7.x
Development

Successfully merging this pull request may close these issues.

Add support to log hparams and metrics to tensorboard?
6 participants
@justusschock @festeh @Borda @pep8speaks @olineumann @williamFalcon