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 step param in dvclive.log method #130

Merged
merged 3 commits into from
Aug 2, 2021
Merged

Fix step param in dvclive.log method #130

merged 3 commits into from
Aug 2, 2021

Conversation

naibatsuteki
Copy link
Contributor

Improve conditional expression checking step param value.

Closes #129

… as step.

Now only None value is prohibited.
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Just a minor thing, could you:

  • Add a regression test (test that fails because of the bug and passes after the fix)

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2021

Codecov Report

Merging #130 (ec61992) into master (b24fda7) will not change coverage.
The diff coverage is 100.00%.

❗ Current head ec61992 differs from pull request most recent head 2e81366. Consider uploading reports for the commit 2e81366 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #130   +/-   ##
=======================================
  Coverage   88.69%   88.69%           
=======================================
  Files          11       11           
  Lines         292      292           
=======================================
  Hits          259      259           
  Misses         33       33           
Impacted Files Coverage Δ
dvclive/metrics.py 94.39% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b24fda7...2e81366. Read the comment docs.

Crucial thing is modify internal state before perform log call with 0 as step value.
tests/test_main.py Outdated Show resolved Hide resolved
@daavoo daavoo merged commit 06b7660 into iterative:master Aug 2, 2021
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.

metrics bug: dvclive.log with 0 as step param value, save wrong step to log file
3 participants