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

Convert progress bar metrics to float #5692

Merged
merged 5 commits into from
Feb 11, 2021
Merged

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Jan 28, 2021

What does this PR do?

Fixes #5481

### TODO:
- Separate train/val/test metrics
- callback_metrics should copy the data as is, not as a reference from progress_bar_metrics. This way, each MetricHolder is free to make their own conversions without affecting other parts.

edit: leaving refactor out of this PR

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • [n/a] Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
  • Check that target branch and milestone match!

@carmocca carmocca self-assigned this Jan 28, 2021
@carmocca carmocca added bug Something isn't working logging Related to the `LoggerConnector` and `log()` refactor labels Jan 28, 2021
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #5692 (f38d7cb) into release/1.2-dev (a028171) will decrease coverage by 44%.
The diff coverage is 95%.

@@                Coverage Diff                @@
##           release/1.2-dev   #5692     +/-   ##
=================================================
- Coverage               88%     44%    -44%     
=================================================
  Files                  181     181             
  Lines                12964   12796    -168     
=================================================
- Hits                 11462    5632   -5830     
- Misses                1502    7164   +5662     

@Borda
Copy link
Member

Borda commented Jan 29, 2021

@carmocca is it still WIP or ready to go? :]

@carmocca
Copy link
Contributor Author

carmocca commented Jan 29, 2021

So I planned to include a larger refactor here. I think i'll commit the one-line fix to master and work on the refactor here.

edit: Nevermind, MetricHolder is not in master

@Borda
Copy link
Member

Borda commented Jan 29, 2021

So I planned to include a larger refactor here. I think i'll commit the one-line fix to master and work on the refactor here.

I would rather have multiple smaller refactors, it is easier to review :]

@carmocca carmocca added this to the 1.2 milestone Feb 3, 2021
@carmocca carmocca removed the refactor label Feb 3, 2021
@carmocca carmocca marked this pull request as ready for review February 3, 2021 01:30
@carmocca carmocca added the ready PRs ready to be merged label Feb 3, 2021
@carmocca carmocca changed the title [WIP] Convert progress bar metrics to float Convert progress bar metrics to float Feb 3, 2021
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

Just one request: could we move the test to the logging tests? Because the conversion doesn't happen in the progress bar therefore I propose to move this test outside the progress bar unit tests.

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls see #5692 (review)

@carmocca
Copy link
Contributor Author

carmocca commented Feb 4, 2021

Just one request: could we move the test to the logging tests? Because the conversion doesn't happen in the progress bar therefore I propose to move this test outside the progress bar unit tests.

I disagree. The purpose of the test is to check that the progress bar correctly displays the number as a float, not a tensor. The fact that the changed code is in the logger connector is independent to the problem this PR is solving. If the logger connector was completely replaced by a different abstraction, I still want to make sure the progress bar does not change.

Tell me if you still want me to change it.

Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

I am not convinced with your argument but it's ok I don't want to block. If we change our mind we can move the test later.

@edenlightning edenlightning removed this from the 1.2 milestone Feb 9, 2021
@Borda Borda enabled auto-merge (squash) February 10, 2021 16:32
@Borda Borda merged commit e8190e8 into release/1.2-dev Feb 11, 2021
@Borda Borda deleted the 5481-pbar-to-float branch February 11, 2021 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logging Related to the `LoggerConnector` and `log()` ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants