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

Add type check on maximize in MetricTracker #1430

Merged

Conversation

ValerianRey
Copy link
Contributor

What does this PR do?

Fixes #1428

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? (No doc change required)
  • Did you write any new necessary tests?

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?

Yes!

* Add test case to `test_raises_error_on_wrong_input` to check that the initialization of a MetricTracker with a single `metric` and a list `maximize` raises a ValueError
* Add check on maximize to be a singe bool when the metric is a single Metric
@ValerianRey ValerianRey changed the title 1428 metrictracker init check Add type check on maximize in MetricTracker Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 7, 2023

Codecov Report

Merging #1430 (5013f64) into master (107dbfd) will decrease coverage by 52%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1430     +/-   ##
========================================
- Coverage      90%     38%    -52%     
========================================
  Files         211     211             
  Lines       10844   10846      +2     
========================================
- Hits         9714    4124   -5590     
- Misses       1130    6722   +5592     

@mergify mergify bot added the ready label Jan 7, 2023
@Borda Borda enabled auto-merge (squash) January 9, 2023 07:26
@Borda Borda merged commit 7b505ff into Lightning-AI:master Jan 9, 2023
Borda pushed a commit that referenced this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MetricTracker of a single metric should not accept a list of maximize
3 participants