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

Use arange and repeat for deterministic bincount #2184

Merged
merged 24 commits into from
Nov 25, 2023

Conversation

kyle-dorman
Copy link
Contributor

@kyle-dorman kyle-dorman commented Oct 17, 2023

What does this PR do?

Fixes #1413

Uses torch.arange and torch.repeat instead of a for loop to deterministically calculate bincount. Should run MUCH faster now.

Before submitting
  • [ Y] Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • [Y ] Did you read the contributor guideline, Pull Request section?
  • [Y ] Did you make sure to update the docs?
  • [N/A ] Did you write any new necessary tests?
PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃


📚 Documentation preview 📚: https://torchmetrics--2184.org.readthedocs.build/en/2184/

Verified

This commit was signed with the committer’s verified signature.
svanharmelen Sander van Harmelen
for more information, see https://pre-commit.ci
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #2184 (b3762a6) into master (1b16341) will increase coverage by 0%.
The diff coverage is 0%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2184   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         293     293           
  Lines       16425   16423    -2     
======================================
  Hits        14279   14279           
+ Misses       2146    2144    -2     

@Borda Borda requested a review from SkafteNicki October 25, 2023 06:28
@Borda Borda enabled auto-merge (squash) October 25, 2023 06:28
@mergify mergify bot added the ready label Oct 25, 2023
@daturkel
Copy link

daturkel commented Nov 8, 2023

Is there a blocker for merging this change? My team would love to incorporate it as the speed of validation metrics is currently slowing down our ability to iterate.

@mergify mergify bot removed the ready label Nov 10, 2023
@SkafteNicki
Copy link
Member

Hi @daturkel, sorry nothing in principal stopping this from being merged, we have just not been around for the last couple of weeks. Trying to pick up the work now.
There is a problem with our CI that is currently blocking all PRs. When that is fixed, this should hopefully get merged.

@SkafteNicki SkafteNicki added the enhancement New feature or request label Nov 14, 2023
@SkafteNicki SkafteNicki added this to the v1.2.x milestone Nov 14, 2023
@daturkel
Copy link

Hi @daturkel, sorry nothing in principal stopping this from being merged, we have just not been around for the last couple of weeks. Trying to pick up the work now. There is a problem with our CI that is currently blocking all PRs. When that is fixed, this should hopefully get merged.

All good @SkafteNicki , I appreciate the team's effort! Thanks for the update

@kyle-dorman kyle-dorman changed the title Use meshgrid for deterministic bincount Use arange and repeat for deterministic bincount Nov 14, 2023
update changelog
auto-merge was automatically disabled November 14, 2023 16:15

Head branch was pushed to by a user without write access

update comment
@mergify mergify bot added ready and removed has conflicts labels Nov 21, 2023
@SkafteNicki SkafteNicki enabled auto-merge (squash) November 25, 2023 15:20
@SkafteNicki SkafteNicki merged commit 8ef2a89 into Lightning-AI:master Nov 25, 2023
Borda pushed a commit that referenced this pull request Nov 30, 2023
* Use meshgrid for deterministic bincount

* Update src/torchmetrics/utilities/data.py

Use arange instead of meashgrid

* Update data.py

Update _bincount doc

* chlog

* size

* Update CHANGELOG.md

update changelog

* Update data.py

update comment

* improve text

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

(cherry picked from commit 8ef2a89)
Borda pushed a commit that referenced this pull request Dec 1, 2023
* Use meshgrid for deterministic bincount

* Update src/torchmetrics/utilities/data.py

Use arange instead of meashgrid

* Update data.py

Update _bincount doc

* chlog

* size

* Update CHANGELOG.md

update changelog

* Update data.py

update comment

* improve text

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

(cherry picked from commit 8ef2a89)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation with torchmetrics extremely slow
5 participants