Skip to content

Comments

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

Merged
vfdev-5 merged 11 commits intopytorch:masterfrom
DeepC004:deepc004/issue#2853
Feb 13, 2023
Merged

Issue#2853: Update once_event_filter in events.py#2858
vfdev-5 merged 11 commits intopytorch:masterfrom
DeepC004:deepc004/issue#2853

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