Skip to content

Add more Events for MONAI workflow engines#1474

Merged
Nic-Ma merged 10 commits intoProject-MONAI:masterfrom
Nic-Ma:add-prepare-loss
Jan 27, 2021
Merged

Add more Events for MONAI workflow engines#1474
Nic-Ma merged 10 commits intoProject-MONAI:masterfrom
Nic-Ma:add-prepare-loss

Conversation

@Nic-Ma
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma commented Jan 20, 2021 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

wyli
wyli requested changes Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
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

@Nic-Ma
[DLMED] add 3 Events
25623cb 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Nic-Ma commented Jan 21, 2021

/black

@Nic-Ma
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

vfdev-5 commented Jan 21, 2021 •
edited
Loading

@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
Copy Markdown
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
Copy Markdown
Member

vfdev-5 commented Jan 21, 2021

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

wyli
wyli reviewed Jan 21, 2021
View reviewed changes
wyli
wyli reviewed Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
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

@Nic-Ma
Copy link
Copy Markdown
Contributor Author

Nic-Ma commented Jan 22, 2021

/black

@Nic-Ma
Copy link
Copy Markdown
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.

wyli
wyli reviewed Jan 26, 2021
View reviewed changes
wyli
wyli approved these changes Jan 27, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Nic-Ma commented Jan 27, 2021 •
edited
Loading

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

Reviewers

@vfdev-5 vfdev-5 vfdev-5 left review comments

@wyli wyli wyli approved these changes

@yanchengnv yanchengnv Awaiting requested review from yanchengnv

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Nic-Ma @vfdev-5 @wyli @monai-bot