Merged
Conversation
vfdev-5
reviewed
Nov 14, 2021
| param_name: name of parameter to update. | ||
| save_history: whether to log the parameter values to | ||
| `engine.state.param_history`, (default=False). | ||
| create_new: in case `param_name` already exists in `engine.state`, whether to authorize `StateParamScheduler` |
Collaborator
There was a problem hiding this comment.
@fco-dv please use double-ticks ```` instead of single-tick when addresses code. Here and above.
Suggested change
| create_new: in case `param_name` already exists in `engine.state`, whether to authorize `StateParamScheduler` | |
| create_new: in case ``param_name`` already exists in ``engine.state``, whether to authorize ``StateParamScheduler`` |
| f"This may be a conflict between multiple StateParameterScheduler handlers." | ||
| f"Please choose another name." | ||
| ) | ||
| if not self.create_new: |
Collaborator
There was a problem hiding this comment.
I do not quite understand this if/else clauses, especially "else" one.
I was thinking about the following logic:
param_nameNOT inengine.stateAND create_new is True => simply create new attributeparam_nameNOT inengine.stateAND create_new is False => warn that we will create new attribute, but to remove this warning, create_new should be Trueparam_nameinengine.stateAND create_new is True => raise error as we can not create new attribute as it already exists in the state.param_nameinengine.stateAND create_new is False => silently override existing attribute
What do you think ?
Contributor
Author
There was a problem hiding this comment.
Oh yes , I've misunderstood the case 3, thought we were handling the creation of state parameter in that case ... So yes Indeed, it's better and simpler ! Thanks
vfdev-5
reviewed
Nov 15, 2021
vfdev-5
reviewed
Nov 15, 2021
Co-authored-by: vfdev <[email protected]>
Co-authored-by: vfdev <[email protected]>
vfdev-5
approved these changes
Nov 15, 2021
Comment on lines
394
to
413
| def test_param_scheduler_with_ema_handler_attach_exception(): | ||
| import torch.nn as nn | ||
|
|
||
| from ignite.handlers import EMAHandler | ||
|
|
||
| data = torch.rand(100, 2) | ||
| model = nn.Linear(2, 1) | ||
| trainer = Engine(lambda e, b: model(b)) | ||
| param_name = "ema_decay" | ||
| save_history = True | ||
| create_new = True | ||
|
|
||
| ema_handler = EMAHandler(model) | ||
| ema_handler.attach(trainer, name=param_name, event=Events.ITERATION_COMPLETED) | ||
| ema_decay_scheduler = PiecewiseLinearStateScheduler( | ||
| param_name=param_name, | ||
| milestones_values=[(0, 0.0), (10 * len(data), 0.999)], | ||
| save_history=save_history, | ||
| create_new=create_new, | ||
| ) |
Collaborator
There was a problem hiding this comment.
I think you do not need to add EMAHandler to check ValueError.
Collaborator
There was a problem hiding this comment.
Can we add a working test with EMAHandler and PiecewiseLinearStateScheduler like in the issue ?
Contributor
Author
There was a problem hiding this comment.
Yes sure , thanks for your comments @vfdev-5 !
vfdev-5
reviewed
Nov 18, 2021
fco-dv
added a commit
to fco-dv/ignite
that referenced
this pull request
Nov 23, 2021
* pytorch#2090 make work EMAHandler with StateParamScheduler * pytorch#2295 update docstring for parameter `create_new` * pytorch#2295 fixing attach method logic and update associated tests * pytorch#2295 fix unused import / remove useless test * pytorch#2295 fix tests * pytorch#2295 rm else statement * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * pytorch#2295 update tests assert messages * pytorch#2295 update tests, add working example with EMAHandler Co-authored-by: vfdev <[email protected]>
Ishan-Kumar2
pushed a commit
to Ishan-Kumar2/ignite
that referenced
this pull request
Dec 26, 2021
* pytorch#2090 make work EMAHandler with StateParamScheduler * pytorch#2295 update docstring for parameter `create_new` * pytorch#2295 fixing attach method logic and update associated tests * pytorch#2295 fix unused import / remove useless test * pytorch#2295 fix tests * pytorch#2295 rm else statement * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * pytorch#2295 update tests assert messages * pytorch#2295 update tests, add working example with EMAHandler Co-authored-by: vfdev <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2295
Description:
Add
create_newparameter toattachmethod . This flag is used to createparam_nameonengine.stateand is taking into account whetherparam_nameattribute already exists or not onengine.state. Overrides existing attribute by default.Check list: