Skip to content

Feature fixed weights hist handler issue 2328#2523

Merged
vfdev-5 merged 36 commits intopytorch:masterfrom
sadra-barikbin:Feature-weights-hist-handler-issue-2328
Apr 11, 2022
Merged

Feature fixed weights hist handler issue 2328#2523
vfdev-5 merged 36 commits intopytorch:masterfrom
sadra-barikbin:Feature-weights-hist-handler-issue-2328

Conversation

@sadra-barikbin
Copy link
Copy Markdown
Collaborator

@sadra-barikbin sadra-barikbin commented Mar 22, 2022 •
edited by vfdev-5
Loading

Fixes #2328

Description:

  • Added whitelist arg to WeightsHistHandler to be able to log specific weights to TensorboardLogger.

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)

sadra-barikbin and others added 25 commits January 22, 2022 23:03
@sadra-barikbin
Remove unnecessary code in BaseOutputHandler
3484a59 
Closes pytorch#2438
@sadra-barikbin
Merge branch 'pytorch:master' into master
2c8eed9
@sadra-barikbin
Add ReduceLROnPlateauScheduler
ccf2364 
Closes pytorch#1754
@sadra-barikbin
Fix indentation issue
7f7dae6
@sadra-barikbin
Fix another indentation issue
896e482
@sadra-barikbin
Fix PEP8 related issues
cbc8d04
@sadra-barikbin
Fix other PEP8 related issues
47b0622
@sadra-barikbin
Fix hopefully the last PEP8 related issue
91d058e
@sadra-barikbin
Fix hopefully the last PEP8 related issue
9fd7d61
@sadra-barikbin
Merge branch 'pytorch:master' into master
b7dc921
@sadra-barikbin
Merge branch 'master' of https://github.com/sadra-barikbin/ignite
e0644e3
@sadra-barikbin
Remove ReduceLROnPlateau's specific params and add link to it
c95a2be 
Also fix bug in min_lr check
@sadra-barikbin
Fix state_dict bug and add a test
96554d0
@sadra-barikbin
Merge branch 'pytorch:master' into master
145dabc
@sadra-barikbin
Update docs
0aee28a
@vfdev-5
Merge branch 'master' into master
307803c
@sadra-barikbin
Merge branch 'pytorch:master' into master
0129572
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
a17a5b2
@sadra-barikbin
Add doctest and fix typo
b3ea962
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
e2e6831
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
b88c9e1
@sadra-barikbin
Merge branch 'master' of https://github.com/sadra-barikbin/ignite
8d0ae3c
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
102ba25
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
a5d9bab
@sadra-barikbin
Merge branch 'master' of https://github.com/pytorch/ignite
8ad4113
@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch from 59ea6bf to acfa809 Compare March 22, 2022 16:59
@github-actions github-actions bot added the module: contrib Contrib module label Mar 22, 2022
@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Mar 22, 2022

@sadra-barikbin thanks for the PR, the issue was to improve current WeightsHistHandler without introducing another class.

@sadra-barikbin
Copy link
Copy Markdown
Collaborator Author

sadra-barikbin commented Mar 22, 2022

Oh. So I add a whitelist parameter to WeightsHistHandler. If it was None, I log all of weights, otherwise just specified weights. Is this OK?

@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch from bd9cd32 to 2a40efb Compare March 22, 2022 21:45
@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch from 42e00db to f858e17 Compare March 26, 2022 19:17
vfdev-5
vfdev-5 reviewed Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vfdev-5 vfdev-5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR @sadra-barikbin
I left few comments on to improve the code. Let me know what do you think

@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch 3 times, most recently from 3dd8ece to 6ac8394 Compare April 4, 2022 23:08
vfdev-5
vfdev-5 reviewed Apr 5, 2022
View reviewed changes
@vfdev-5 vfdev-5 requested a review from sdesrozis April 5, 2022 11:14
@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Apr 5, 2022

@sadra-barikbin thanks for the updates ! Can you now run this example with specifying whitelist=["conv", ] here:

ignite/examples/contrib/mnist/mnist_with_tensorboard_logger.py

Line 136 in 82a929d

tb_logger.attach(trainer, log_handler=WeightsScalarHandler(model), event_name=Events.ITERATION_COMPLETED(every=100))

and report here the result ?

sdesrozis
sdesrozis approved these changes Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@sdesrozis sdesrozis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@sadra-barikbin Thanks very much, LGTM ! Although a comment still to be addressed.

@sadra-barikbin
Copy link
Copy Markdown
Collaborator Author

sadra-barikbin commented Apr 10, 2022

Hi, I ran the example. Result is depicted in picture below:

image

By the way, there was mistakes in lines 138 and 142 in event_name parameter.

@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch from b800c83 to 091a54f Compare April 10, 2022 23:58
@sadra-barikbin sadra-barikbin force-pushed the Feature-weights-hist-handler-issue-2328 branch from 091a54f to f57b890 Compare April 11, 2022 01:52
vfdev-5
vfdev-5 approved these changes Apr 11, 2022
View reviewed changes
Copy link
Copy Markdown
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 @sadra-barikbin !

@vfdev-5 vfdev-5 merged commit 7e05d91 into pytorch:master Apr 11, 2022
@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Apr 11, 2022

@sadra-barikbin can you add please a similar arg to other weights and grad logging handlers ?

@sadra-barikbin
Copy link
Copy Markdown
Collaborator Author

sadra-barikbin commented Apr 11, 2022

Sure. Shall I open a new issue for them? You mean just Tensorboard ones?

@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Apr 11, 2022 •
edited
Loading

yes, please open an issue and suggest the classes where we could add such new argument.
By the way, I think we forgot about docs tag for new args in this PR: .. versionchanged:: (check the codebase for examples of usage)
Please send a follow-up update. Thanks

@sadra-barikbin sadra-barikbin deleted the Feature-weights-hist-handler-issue-2328 branch April 13, 2022 21:45
@sadra-barikbin sadra-barikbin restored the Feature-weights-hist-handler-issue-2328 branch April 13, 2022 21:45
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 approved these changes

+1 more reviewer

@sdesrozis sdesrozis sdesrozis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: contrib Contrib module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

WeightsHistHandler should plot all weights (incl those without grad)

3 participants

@sadra-barikbin @vfdev-5 @sdesrozis