Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for pytorch 1.6.0 #653

Merged
merged 22 commits into from
Aug 14, 2020
Merged

Updates for pytorch 1.6.0 #653

merged 22 commits into from
Aug 14, 2020

Conversation

mtar
Copy link
Collaborator

@mtar mtar commented Aug 7, 2020

Description

Update the unittests for argmax and argmin for the changed return behaviour (numpy like) resulting from changes in pytorch 1.6.0 (torch.max, torch.min return first value).
Additionally, mpi_argmax and mpi_argmin sort the inputs by starting index beforehand. Some MPI implementations do not bind the first input to the lower process.

Changes proposed:

  • argmax test update
  • argmin test update
  • requires torch 1.6.0
  • sort arguments in mpi_argmax, mpi_argmin (Fix bug in some MPI implementations)
  • updated gpu calls in a few tests (see code for details)
  • io tests use the current working directory for the tests instead of a temp directory

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

Unknown

@mtar mtar marked this pull request as ready for review August 7, 2020 14:25
@mtar mtar requested a review from coquelin77 August 7, 2020 14:25
@mtar mtar changed the title Fix unittests argmax argmin Fix unittests argmax argmin with pytorch 1.6.0 Aug 7, 2020
@mtar mtar mentioned this pull request Aug 7, 2020
4 tasks
@coquelin77
Copy link
Member

these tests will not run on GPU. is there a reason that we cant use the torch argmax/argmin for the tests?

@mtar mtar marked this pull request as draft August 11, 2020 05:28
@mtar mtar closed this Aug 11, 2020
@mtar mtar reopened this Aug 12, 2020
coquelin77
coquelin77 previously approved these changes Aug 13, 2020
@mtar mtar marked this pull request as ready for review August 13, 2020 08:57
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #653 into master will decrease coverage by 0.02%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   97.40%   97.37%   -0.03%     
==========================================
  Files          85       85              
  Lines       16636    16646      +10     
==========================================
+ Hits        16204    16209       +5     
- Misses        432      437       +5     
Impacted Files Coverage Δ
heat/cluster/tests/test_kmedoids.py 100.00% <ø> (ø)
heat/core/printing.py 98.41% <75.00%> (-1.59%) ⬇️
heat/core/statistics.py 96.76% <77.77%> (-0.56%) ⬇️
heat/cluster/tests/test_spectral.py 100.00% <100.00%> (ø)
heat/core/_operations.py 92.73% <100.00%> (ø)
heat/core/dndarray.py 97.22% <100.00%> (-0.01%) ⬇️
heat/core/linalg/basics.py 94.46% <100.00%> (ø)
heat/core/manipulations.py 99.12% <100.00%> (ø)
heat/core/tests/test_dndarray.py 100.00% <100.00%> (ø)
heat/core/tests/test_io.py 93.03% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7616ca8...cfb588b. Read the comment docs.

@coquelin77 coquelin77 requested a review from Cdebus August 13, 2020 12:52
@coquelin77
Copy link
Member

build failing -> due to issues with jenkins
codecov -> most changes are within aspects which are not touched (5 lines) by travis, 4 in MPI operations, 1 is a print change for old torch versions

@coquelin77 coquelin77 changed the title Fix unittests argmax argmin with pytorch 1.6.0 Updates for pytorch 1.6.0 Aug 14, 2020
Copy link
Contributor

@Cdebus Cdebus left a comment

Choose a reason for hiding this comment

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

Looks good to me and tests are passing

@Cdebus Cdebus merged commit 75014ca into master Aug 14, 2020
@Cdebus Cdebus deleted the bug/argmin_argmax_unittests branch August 14, 2020 12:04
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