Introduce a variable skip_unrolling in class Metric#3258
Introduce a variable skip_unrolling in class Metric#3258vfdev-5 merged 14 commits intopytorch:masterfrom
Conversation
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks for the PR @simeetnayan81
Left a comment about the implementation
ignite/metrics/metric.py
Outdated
|
|
||
| def __init__( | ||
| self, output_transform: Callable = lambda x: x, device: Union[str, torch.device] = torch.device("cpu") | ||
| self, output_transform: Callable = lambda x: x, device: Union[str, torch.device] = torch.device("cpu"), skip_unrolling : bool =False |
There was a problem hiding this comment.
Let's also update above docstring by adding the new argument. Please also check CONTRIBUTING guideline the part about how to add .. versionadded:: tag in the bottom of the docstring. Version to put should be 0.5.1
There was a problem hiding this comment.
Should I add .. versionchanged:: instead?
There was a problem hiding this comment.
Yes, correct, it should be versionchanged tag, https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#writing-documentation
There was a problem hiding this comment.
On it. Thanks.
There was a problem hiding this comment.
Have made the changes. Kindly review.
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks for the update! I added few minor comments. Let's add new feature tests and run the CI to see if any failures
Co-authored-by: vfdev <[email protected]>
Co-authored-by: vfdev <[email protected]>
|
Tests should be added to end of the test_metric.py file? |
|
Yes, you can add it in the end of the file |
|
|
vfdev-5
left a comment
There was a problem hiding this comment.
Looks good @simeetnayan81 , thanks, let's just add an example of usage of the flag in the docstring and it will be good to go, once CI is green (except unrelated failures)
|
@vfdev-5 Before adding the example in the docstring, I wanted to confirm, to make skip_unrolling effective for the loss function, we might also need to change this. Change to: |
|
@simeetnayan81 yes, you are right, we need to add this new arg to all metrics defining a constructor. Let's update Loss metric here and update other metrics in a follow-up PR. |
… skip_unrolling arg
|
Things to do in a follow-up PR.
|
|
Thanks for the updates and the TODO. Can we do this point here ?
|
|
Alright @vfdev-5 |
|
Have made the changes, the new test works locally. |
|
The test is failing because |
vfdev-5
left a comment
There was a problem hiding this comment.
Thanks @simeetnayan81 , lgtm
Fixes #2940
Description:
Introduce a variable skip_unrolling in class Metric as discussed here https://discord.com/channels/831462531327328276/1110662056622964860/1253769540710567977
Check list: