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 more Events for MONAI workflow engines #1474

Merged
merged 10 commits into from
Jan 27, 2021

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Jan 20, 2021

Description

This PR registered more Events to ignite engine for MONAI workflow trainer and evaluator.
Users can write more powerful handlers based on it.
If user has very special iteration computation, they can still use iteration_update() callback.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh --codeformat --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@Nic-Ma Nic-Ma requested review from wyli and yanchengnv January 20, 2021 03:29
@Nic-Ma Nic-Ma changed the title [WIP] Add prepare_loss callable API for engines [WIP] Add more Events for MONAI workflow engines Jan 21, 2021
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

@Nic-Ma Nic-Ma marked this pull request as ready for review January 21, 2021 08:56
@Nic-Ma Nic-Ma changed the title [WIP] Add more Events for MONAI workflow engines Add more Events for MONAI workflow engines Jan 21, 2021
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks this needs more discussions and tests

Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

monai-bot and others added 2 commits January 21, 2021 09:42
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

Hi @wyli and @yanchengnv ,

Thanks for your reminder, I added test cases in the integration tests.
I think it's ready for review now.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

And BTW, this PR refers to ignite example: https://github.com/pytorch/ignite/blob/master/ignite/engine/events.py#L146
Thanks for @vfdev-5 's help.

@vfdev-5
Copy link
Member

vfdev-5 commented Jan 21, 2021

@Nic-Ma thanks for mentioning ! Just FYI, currently we have undocumented requirements for custom events if using filters with them: pytorch/ignite#1555
Hopefully, docs issue will be fixed in the next release v0.4.3 and in v0.5.0 user wont need to increment state attribute counter:

def train_step(engine, batch):
    engine.fire_event(TestEvents.TEST_EVENT)
    engine.state.test_event += 1

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 21, 2021

Hi @vfdev-5 ,

Thanks for your tips, I didn't add event_to_attr dict to the register_events() call because I think we don't have count attributes to keep in State for these 3 Events. So we don't need engine.state.test_event += 1, right?

Thanks.

@vfdev-5
Copy link
Member

vfdev-5 commented Jan 21, 2021

Yes, correct, in this case you do not need engine.state.test_event += 1.

monai/engines/trainer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks please see some comments inline, we'll discuss this next monday

monai/engines/utils.py Outdated Show resolved Hide resolved
monai/engines/workflow.py Show resolved Hide resolved
monai/engines/workflow.py Outdated Show resolved Hide resolved
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 22, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 22, 2021

Hi @wyli ,

Thanks for your review.
I updated the PR according to your comments.
Could you please help review it again?

Thanks.

Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks, this PR adds a few events to each iteration. just wanted to note that state.output is used to cache information across those events. not sure if this is the best way of using state.output but seems like a viable workaround according to @Nic-Ma

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Jan 27, 2021

Thanks @wyli ,

And @vfdev-5 , as we discussed in the above thread, I think saving data to engine.state.output is acceptable solution so far, referring to ignite contrib: https://github.com/pytorch/ignite/blob/master/ignite/contrib/engines/tbptt.py#L111.
But please let me know if this design is against your ignite design or any future plan, then we can update it.

Thanks.

@Nic-Ma Nic-Ma merged commit 8207e1e into Project-MONAI:master Jan 27, 2021
@Nic-Ma Nic-Ma deleted the add-prepare-loss branch July 2, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants