Skip to content

Conversation

harsh8398
Copy link
Contributor

Completes #1427

Description:

  • Added example notebook showing usage of HandlerTimeProfiler
  • Added notebook entry in README as well

@harsh8398
Copy link
Contributor Author

Just want to add that, while modifying README I noticed that pre-commit was suggesting lot of whitespace related changes in the README.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Nov 22, 2020

@harsh8398 thanks for the PR ! I was wondering if you also see the cell with printed results like below:
Screen Shot 2020-11-22 at 15 18 42

Just want to add that, while modifying README I noticed that pre-commit was suggesting lot of whitespace related changes in the README.

About the precommit, I think it should be related to #1468. Could you please dump the diff once with the precommit, maybe it is OK and just accept them ?

@harsh8398
Copy link
Contributor Author

harsh8398 commented Nov 22, 2020

@vfdev-5 Yes, I am seeing the same output. I was able to fix that spill by using the trick mentioned here. The output looks as follows now:
image

Could you please dump the diff once with the precommit, maybe it is OK and just accept them ?

Sorry, I didn't get you. What are you suggesting? Do you want me to add those precommit changes in this PR?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 3, 2020

@sdesrozis could you please review the PR and merge if it is OK

@sdesrozis
Copy link
Contributor

sdesrozis commented Dec 3, 2020

@harsh8398 Maybe we should add a line to install Pytorch-ignite in notebook

!pip install --upgrade --pre pytorch-ignite

What do you think ?

There is a warning from ignite/contrib/handlers/time_profilers.py:587

UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
  data = torch.tensor(data, dtype=torch.float32)

Could we remove it ?

EDIT : mentioned points are not mandatory and should be addressed in PRs (others notebooks don't have install command and warning is not directly related to this work).

@harsh8398
Copy link
Contributor Author

@sdesrozis I have added the suggested cell at the top. Also, I've raised a PR(#1487) for the change required to remove the warning.

@sdesrozis
Copy link
Contributor

@harsh8398 I think you didn't commit the good installation command line in notebook. Could you check and fix please ?

@harsh8398
Copy link
Contributor Author

@sdesrozis Sorry for that, Its fixed now.

@sdesrozis
Copy link
Contributor

@harsh8398 Thank you LGTM !

Copy link
Contributor

@sdesrozis sdesrozis left a comment

Choose a reason for hiding this comment

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

Thanks

@vfdev-5 vfdev-5 merged commit 10eccdb into pytorch:master Dec 6, 2020
@harsh8398 harsh8398 deleted the handlers_profiler_example branch December 6, 2020 12:33
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.

3 participants