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 bug in metric tracker #1306

Merged
merged 5 commits into from
Nov 3, 2022
Merged

Fix bug in metric tracker #1306

merged 5 commits into from
Nov 3, 2022

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Oct 31, 2022

What does this PR do?

Fixes #1304
Wrong variable was returned in the return_step=False case due to confusing variable naming.

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?

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 🙃

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Oct 31, 2022
@SkafteNicki SkafteNicki added this to the v0.10 milestone Oct 31, 2022
@SkafteNicki SkafteNicki changed the title fix + test Fix bug in metric tracker Oct 31, 2022
@SkafteNicki SkafteNicki enabled auto-merge (squash) October 31, 2022 19:21
@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@669bca0). Click here to learn what that means.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##             master   #1306   +/-   ##
========================================
  Coverage          ?     37%           
========================================
  Files             ?     190           
  Lines             ?   11120           
  Branches          ?       0           
========================================
  Hits              ?    4152           
  Misses            ?    6968           
  Partials          ?       0           

@mergify mergify bot added the ready label Oct 31, 2022
@stancld
Copy link
Contributor

stancld commented Nov 2, 2022

Our integration tests stared failing:

C:\hostedtoolcache\windows\Python\3.10.8\x64\lib\site-packages\pytorch_lightning\trainer\trainer.py:579: in fit
    call._call_and_handle_interrupt(
C:\hostedtoolcache\windows\Python\3.10.8\x64\lib\site-packages\pytorch_lightning\trainer\call.py:38: in _call_and_handle_interrupt
    return trainer_fn(*args, **kwargs)
C:\hostedtoolcache\windows\Python\3.10.8\x64\lib\site-packages\pytorch_lightning\trainer\trainer.py:621: in _fit_impl
    self._run(model, ckpt_path=self.ckpt_path)
C:\hostedtoolcache\windows\Python\3.10.8\x64\lib\site-packages\pytorch_lightning\trainer\trainer.py:984: in _run
    verify_loop_configurations(self)
C:\hostedtoolcache\windows\Python\3.10.8\x64\lib\site-packages\pytorch_lightning\trainer\configuration_validator.py:55: in verify_loop_configurations
    _check_on_epoch_start_end(model)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

model = TestModel(
  (layer): Linear(in_features=32, out_features=2, bias=True)
  (metric_step): SumMetric()
  (metric_epoch): SumMetric()
)

    def _check_on_epoch_start_end(model: "pl.LightningModule") -> None:
        hooks = (
            ("on_epoch_start", "on_<train/validation/test>_epoch_start"),
            ("on_epoch_end", "on_<train/validation/test>_epoch_end"),
        )
    
        for hook, alternative_hook in hooks:
            if callable(getattr(model, hook, None)):
>               raise RuntimeError(
                    f"The `LightningModule.{hook}` hook was removed in v1.8. Please use"
                    f" `LightningModule.{alternative_hook}` instead."
                )
E               RuntimeError: The `LightningModule.on_epoch_start` hook was removed in v1.8. Please use `LightningModule.on_<train/validation/test>_epoch_start` instead.

@Borda @justusschock

---> #1308

@SkafteNicki SkafteNicki merged commit 8fc6de8 into master Nov 3, 2022
@SkafteNicki SkafteNicki deleted the bugfix/tracker branch November 3, 2022 11:20
Borda pushed a commit that referenced this pull request Nov 16, 2022
* fix + test

* changelog

(cherry picked from commit 8fc6de8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MetricTracker best_metric() returns the step instead of the best value
4 participants