Skip to content

Issue#2853: Update once_event_filter in events.py #2858

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

Merged
merged 11 commits into from
Feb 13, 2023

Conversation

DeepC004
Copy link
Contributor

@DeepC004 DeepC004 commented Feb 10, 2023

Related to #2853

Description:

  • Updated once_event_filter in events.py

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)

@github-actions github-actions bot added the module: engine Engine module label Feb 10, 2023
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.

@DeepC004 thanks for the PR, please add unit tests

@DeepC004
Copy link
Contributor Author

@vfdev-5 thank you for reviewing the PR. I will make the changes as stated above and add unit tests.

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 for the updates @DeepC004 !
I left few other comments on how to improve this PR.

@DeepC004 DeepC004 marked this pull request as draft February 10, 2023 19:34
@DeepC004 DeepC004 marked this pull request as ready for review February 11, 2023 14:10
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.

LGTM, thanks @DeepC004 !

I started the CI. Once required tests are green we can merge the PR

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 11, 2023

@DeepC004 there are code formatting issues:

 + flake8 ignite tests examples --config setup.cfg
ignite/engine/events.py:98:121: E501 line too long (129 > 120 characters)
tests/ignite/engine/test_custom_events.py:156:121: E501 line too long (134 > 120 characters)
tests/ignite/engine/test_custom_events.py:159:121: E501 line too long (134 > 120 characters)
tests/ignite/engine/test_custom_events.py:161:1: W293 blank line contains whitespace
tests/ignite/engine/test_custom_events.py:162:121: E501 line too long (134 > 120 characters)
tests/ignite/engine/test_custom_events.py:434:9: F811 redefinition of unused 'assert_once' from line 415
tests/ignite/engine/test_custom_events.py:437:1: W293 blank line contains whitespace
tests/ignite/engine/test_custom_events.py:439:9: F811 redefinition of unused 'assert_' from line 420
tests/ignite/engine/test_custom_events.py:449:5: E303 too many blank lines (2)

Please run the following locally to fix it:

  bash ./tests/run_code_style.sh install
  bash ./tests/run_code_style.sh fmt

However, the following errors should be fixed manually:

tests/ignite/engine/test_custom_events.py:434:9: F811 redefinition of unused 'assert_once' from line 415
tests/ignite/engine/test_custom_events.py:439:9: F811 redefinition of unused 'assert_' from line 420

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 13, 2023

@DeepC004 please run bash ./tests/run_code_style.sh fmt to fix code formatting issue

@DeepC004
Copy link
Contributor Author

DeepC004 commented Feb 13, 2023

@DeepC004 please run bash ./tests/run_code_style.sh fmt to fix code formatting issue

I ran the code for formatting the files. The issue is length of some lines is greater than the maximum line length limit. It turns out to be a line for returning the error comment, which I believe can be written in more concise manner.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 13, 2023

Let me restart the CI by closing and reopening the PR

@vfdev-5 vfdev-5 closed this Feb 13, 2023
@vfdev-5 vfdev-5 reopened this Feb 13, 2023
@vfdev-5 vfdev-5 added this pull request to the merge queue Feb 13, 2023
Merged via the queue into pytorch:master with commit 531e118 Feb 13, 2023
@DeepC004
Copy link
Contributor Author

@vfdev-5 thanks for helping me out throughout the PR. I will remember to consider small details like line length limit and junk whitespaces while contributing. This was my first merged PR, sorry incase I bugged you with doubts and code errors :D

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 14, 2023

@DeepC004 thanks for your contribution and no worries about code formatting, this can be painful in the beginning ! Hope to see more of your contributions 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: engine Engine module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants