Skip to content

fix minor ASAN issues in UMAPAlgo::Optimize::find_params_ab() - #4405

Merged
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-22.02from
yitao-li:bugfix/umap-find-params-ab-asan-issue
Dec 6, 2021
Merged

fix minor ASAN issues in UMAPAlgo::Optimize::find_params_ab()#4405
rapids-bot[bot] merged 2 commits into
NVIDIA:branch-22.02from
yitao-li:bugfix/umap-find-params-ab-asan-issue

Conversation

@yitao-li

@yitao-li yitao-li commented Nov 24, 2021

Copy link
Copy Markdown

There were actuall 2 minor issues that prevented UMAPAlgo::Optimize::find_params_ab() from being ASAN-clean at the moment:

  • One is the mem leaks, of course
  • Another one is the malloc()-delete mismatch -- only memory allocated using new or equivalent should be freed with operator delete or delete[]

Another issue that was also addressed here: exception safety (i.e., by using make_unique from C++-14)

Signed-off-by: Yitao Li yitao@rstudio.com

@yitao-li
yitao-li requested a review from a team as a code owner November 24, 2021 18:02
@GPUtester

Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

Signed-off-by: Yitao Li <yitao@rstudio.com>
@yitao-li
yitao-li force-pushed the bugfix/umap-find-params-ab-asan-issue branch from 499bb3b to 3106f80 Compare November 24, 2021 18:09
@cjnolet cjnolet added bug Something isn't working non-breaking Non-breaking change labels Nov 29, 2021
@cjnolet

cjnolet commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator

ok to test

1 similar comment
@cjnolet

cjnolet commented Nov 29, 2021

Copy link
Copy Markdown
Collaborator

ok to test

Signed-off-by: Yitao Li <yitao@rstudio.com>
@cjnolet

cjnolet commented Dec 2, 2021

Copy link
Copy Markdown
Collaborator

rerun tests

2 similar comments
@cjnolet

cjnolet commented Dec 4, 2021

Copy link
Copy Markdown
Collaborator

rerun tests

@cjnolet

cjnolet commented Dec 6, 2021

Copy link
Copy Markdown
Collaborator

rerun tests

@zbjornson zbjornson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@cjnolet cjnolet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@cjnolet

cjnolet commented Dec 6, 2021

Copy link
Copy Markdown
Collaborator

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 5820133 into NVIDIA:branch-22.02 Dec 6, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…#4405)

There were actuall 2 minor issues that prevented `UMAPAlgo::Optimize::find_params_ab()` from being ASAN-clean at the moment:

- One is the mem leaks, of course
- Another one is the `malloc()`-`delete` mismatch -- only memory allocated using `new` or equivalent should be freed with operator `delete` or `delete[]`

Another issue that was also addressed here: exception safety (i.e., by using `make_unique` from C++-14)

Signed-off-by: Yitao Li <yitao@rstudio.com>

Authors:
  - Yitao Li (https://github.com/yitao-li)

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

URL: NVIDIA#4405
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-22.02@ed0e58c). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.02    #4405   +/-   ##
===============================================
  Coverage                ?   85.83%           
===============================================
  Files                   ?      236           
  Lines                   ?    19410           
  Branches                ?        0           
===============================================
  Hits                    ?    16660           
  Misses                  ?     2750           
  Partials                ?        0           
Flag Coverage Δ
dask 46.42% <ø> (?)
non-dask 78.76% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working CUDA/C++ non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants