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

Adding max_iters as an optional arg in Engine run #1381

Merged
merged 34 commits into from
Nov 9, 2020

Conversation

thescripted
Copy link
Contributor

@thescripted thescripted commented Oct 13, 2020

Fixes #1300

Description:
New tests have not yet been written, and I believe some descriptions/comments will need to be updated. I'm happy to do that, but I'm opening up this PR for feedback and general indications to see if I'm on the right track.

I've assumed that reaching max iteration will fire a "Terminate" event rather than "Epoch Completed," but I don't know if either of those events are accurate for this condition.

FInally, adding a condition to this block:

def _is_done(state: State) -> bool:
return state.iteration == state.epoch_length * state.max_epochs

currently breaks some tests, so I have left that alone for the moment.

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

ignite/engine/engine.py Outdated Show resolved Hide resolved
ignite/engine/engine.py Outdated Show resolved Hide resolved
@sdesrozis
Copy link
Contributor

@thescripted Thank you ! I left few comments.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

@thescripted Thanks for working on the PR !
I left few comment on how to improve it

ignite/engine/engine.py Outdated Show resolved Hide resolved
ignite/engine/engine.py Outdated Show resolved Hide resolved
ignite/engine/engine.py Outdated Show resolved Hide resolved
ignite/engine/engine.py Outdated Show resolved Hide resolved
ignite/engine/engine.py Outdated Show resolved Hide resolved
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 18, 2020

@thescripted could you also please resolve the conflict and update your branch to master.

@thescripted
Copy link
Contributor Author

On it. Sorry for the late reply I've put a bit on my plate with other projects. I'll go ahead and resolve these issues now.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @thescripted ! Looks good

I still have few comments about the PR. For example, a special care is needed here

while self.state.epoch < self.state.max_epochs

if max_epochs is None, right ?

@thescripted
Copy link
Contributor Author

Yes @vfdev-5, I didn't consider that. I believe a fix could be like so:

while not self.should_terminate:  # type: ignore[operator]
    if self.state.max_epochs is not None and self.state.epoch >= self.state.max_epochs:
        break

I also think a condition to check the max_iters here is required.

def _is_done(state: State) -> bool:
return state.iteration == state.epoch_length * state.max_epochs # type: ignore[operator]

From what I can tell max_epochs will never be none, given where and how the method is used Here and Here. I can just add another conditional checking if state.iterations == state.max_iters which may or may not be None.

If those are the right track (and any additional comments you have) I'll be happy to fix those up.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 23, 2020

@thescripted good point about _is_done. So, I'm thinking that we can replace the code

while self.state.epoch < self.state.max_epochs and not self.should_terminate:

by

def _is_done(state: State) -> bool: 
     # something like that
     c1 = state.max_iters and state.iterations == state.max_iters
     c2 = state.epoch_length and state.iteration == state.epoch_length * state.max_epochs     
     return c1 or c2

while not self._is_done(self.state) and not self.should_terminate:

What do you think ?

@vfdev-5 vfdev-5 added the hacktoberfest-accepted For accepted PRs label Oct 25, 2020
vfdev-5 added a commit that referenced this pull request Jun 22, 2021
vfdev-5 added a commit that referenced this pull request Jun 24, 2021
vfdev-5 added a commit that referenced this pull request Jun 24, 2021
trsvchn pushed a commit that referenced this pull request Jul 29, 2021
vfdev-5 added a commit that referenced this pull request Aug 2, 2021
vfdev-5 added a commit that referenced this pull request Oct 11, 2021
vfdev-5 added a commit that referenced this pull request Oct 12, 2021
trsvchn pushed a commit that referenced this pull request Jan 10, 2022
vfdev-5 added a commit that referenced this pull request Jan 13, 2022
vfdev-5 added a commit that referenced this pull request Jan 17, 2022
vfdev-5 added a commit that referenced this pull request May 2, 2022
vfdev-5 added a commit that referenced this pull request May 3, 2022
vfdev-5 added a commit that referenced this pull request May 4, 2022
vfdev-5 added a commit that referenced this pull request Sep 4, 2022
vfdev-5 added a commit that referenced this pull request Sep 4, 2022
vfdev-5 added a commit that referenced this pull request Sep 5, 2022
vfdev-5 added a commit that referenced this pull request Feb 14, 2023
vfdev-5 added a commit that referenced this pull request Feb 17, 2023
vfdev-5 added a commit that referenced this pull request Feb 17, 2023
vfdev-5 added a commit that referenced this pull request Feb 17, 2023
vfdev-5 added a commit that referenced this pull request Feb 18, 2023
vfdev-5 added a commit that referenced this pull request Apr 25, 2023
vfdev-5 added a commit that referenced this pull request Apr 26, 2023
vfdev-5 added a commit that referenced this pull request Oct 2, 2023
vfdev-5 added a commit that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted For accepted PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max_iters as optional arg in Engine run
3 participants