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.

Sign up for GitHub

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

Jump to bottom

[BUG] UMAP test is not built. #3849

Closed
trivialfis opened this issue May 11, 2021 · 8 comments · Fixed by #3848
Closed

[BUG] UMAP test is not built. #3849

trivialfis opened this issue May 11, 2021 · 8 comments · Fixed by #3848
Labels
bug Something isn't working Build or Dep Issues related to building the code or dependencies

Comments

@trivialfis
Copy link
Member

trivialfis commented May 11, 2021 •
edited
Loading

This file https://github.com/rapidsai/cuml/blob/branch-0.20/cpp/test/sg/umap_test.cu is not included in CMakeLists.txt

cuml/cpp/test/CMakeLists.txt

Line 82 in 1ea479b

sg/umap_parametrizable_test.cu

Is this an oversight or an expected behaviour? Or maybe I'm missing something?

The code in this file seems to be outdated.
@trivialfis trivialfis added bug Something isn't working ? - Needs Triage Need team to review and classify labels May 11, 2021
@trivialfis
Copy link
Member Author

If this is unexpected, I can open a PR or just push to #3848 . ;-)

@dantegd
Copy link
Member

dantegd commented May 11, 2021

This seems to have happened a while ago https://github.com/rapidsai/cuml/pull/1985/files in the PR that introduced the parametrizable test (it replaced umap_test for umap_parametrizable_test), and I think it was intended but I could be wrong/misremembering, @viclafargue or @cjnolet can probably answer this better

@mdemoret-nv mdemoret-nv added the Build or Dep Issues related to building the code or dependencies label May 12, 2021
@mdemoret-nv
Copy link
Contributor

@dantegd It might be worthwhile to audit all of the CMakeLists.txt files to make sure no other *.cu/cpp files are missed. Thoughts?

@mdemoret-nv mdemoret-nv removed the ? - Needs Triage Need team to review and classify label May 12, 2021
@dantegd
Copy link
Member

dantegd commented May 12, 2021

@mdemoret-nv good idea, I just went ahead and checked the lists for PR #3844 and the only file missing is this one indeed

@viclafargue
Copy link
Contributor

@dantegd the umap_parametrizable_test.cu file was indeed intended to replace the earlier umap_test.cu file. The new file should cover earlier testing and add some more. The older version was kept in the codebase, but I think that we should be able to safely remove it now.

@mdemoret-nv
Copy link
Contributor

@viclafargue Sounds good.

If this is unexpected, I can open a PR or just push to #3848 . ;-)

Since this is limited to only this file and there is already a UMAP PR ready for review, I'm not opposed to including the fix in PR #3848 and adding this issue to the linked issues in the PR. @dantegd Thoughts? Should we create a new PR or add to the existing?

@dantegd
Copy link
Member

dantegd commented May 12, 2021

Adding to the existing sounds fine for me

@trivialfis
Copy link
Member Author

Pushed into it. Thanks for the replies.

@trivialfis trivialfis mentioned this issue May 13, 2021
Merged
@mdemoret-nv mdemoret-nv linked a pull request May 13, 2021 that will close this issue
[REVIEW] Deterministic UMAP with floating point rounding. #3848
Merged
rapids-bot bot pushed a commit that referenced this issue May 20, 2021
@trivialfis
Deterministic UMAP with floating point rounding. (#3848)
99a80c8 
Use floating rounding to make UMAP optimization deterministic.  This is a breaking change as the batch size parameter is removed.

* Add procedure for rounding the gradient updates.
* Add buffer for gradient updates.
* Add an internal parameter `deterministic`, which should be set to `true` when `random_state` is set.

The test file is removed due to #3849 .

Authors:
  - Jiaming Yuan (https://github.com/trivialfis)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #3848
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this issue Oct 9, 2023
@trivialfis
Deterministic UMAP with floating point rounding. (rapidsai#3848)
cb9fbfe 
Use floating rounding to make UMAP optimization deterministic.  This is a breaking change as the batch size parameter is removed.

* Add procedure for rounding the gradient updates.
* Add buffer for gradient updates.
* Add an internal parameter `deterministic`, which should be set to `true` when `random_state` is set.

The test file is removed due to rapidsai#3849 .

Authors:
  - Jiaming Yuan (https://github.com/trivialfis)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#3848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Build or Dep Issues related to building the code or dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[REVIEW] Deterministic UMAP with floating point rounding. trivialfis/cuml
4 participants
@dantegd @viclafargue @trivialfis @mdemoret-nv