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

Get rid of get_ prefixes #157

Closed
wants to merge 11 commits into from
Closed

Conversation

vighnesh-kadam
Copy link

@vighnesh-kadam vighnesh-kadam commented May 18, 2022

Thank you for contributing to PROJECT :)

Here are some guidelines to help the review process go smoothly.

  1. Please write a description in this text box of the changes that are being
    made.
    Get rid of get_ prefixes

  2. Please ensure that you have written units tests for the changes made/features
    added.

  3. If you are closing an issue please use one of the automatic closing words as
    noted here: https://help.github.com/articles/closing-issues-using-keywords/
    Closes #1478

  4. If your pull request is not ready for review but you want to make use of the
    continuous integration testing facilities please label it with [WIP].

  5. If your pull request is ready to be reviewed without requiring additional
    work on top of it, then remove the [WIP] label (if present) and replace
    it with [REVIEW]. If assistance is required to complete the functionality,
    for example when the C/C++ code of a feature is complete but Python bindings
    are still required, then add the label [HELP-REQ] so that others can triage
    and assist. The additional changes then can be implemented on top of the
    same PR. If the assistance is done by members of the rapidsAI team, then no
    additional actions are required by the creator of the original PR for this,
    otherwise the original author of the PR needs to give permission to the
    person(s) assisting to commit to their personal fork of the project. If that
    doesn't happen then a new PR based on the code of the original PR can be
    opened by the person assisting, which then will be the PR that will be
    merged.

  6. Once all work has been done and review has taken place please do not add
    features or make changes out of the scope of those requested by the reviewer
    (doing this just add delays as already reviewed code ends up having to be
    re-reviewed/it is hard to tell what is new etc!). Further, please do not
    rebase your branch on master/force push/rewrite history, doing any of these
    causes the context of any comments made by reviewers to be lost. If
    conflicts occur against master they should be resolved by merging master
    into the branch used for making the pull request.

Many thanks in advance for your cooperation!

@GPUtester
Copy link

Can one of the admins verify this patch?

@raydouglass
Copy link
Collaborator

okay to test

@vighnesh-kadam
Copy link
Author

Can you Please tell me what mistakes i have made and how i can correct them.

@PointKernel PointKernel changed the title Get rid of get_ prefixes #1478 Get rid of get_ prefixes May 19, 2022
@PointKernel
Copy link
Member

@vighnesh-kadam You renamed all related APIs in include/cuco but didn't change the code in benchmarks, examples, and tests. You may want to build your code locally to catch any leftovers and make sure all code can compile and run.

@jrhemstad jrhemstad added type: feature request New feature request Needs Review Awaiting reviews before merging and removed improvement labels May 19, 2022
@vighnesh-kadam
Copy link
Author

Okay got it ! I will get it done.

@vighnesh-kadam
Copy link
Author

vighnesh-kadam commented May 21, 2022

I am not able to build the repo locally . i keep getting the following error after i run cmake .. command :

CMake Warning:
Ignoring extra path from command line:

".."
CMake Error: The source directory "C:/Users/vighn" does not appear to contain CMakeLists.txt.

@PointKernel
Copy link
Member

PointKernel commented May 24, 2022

I am not able to build the repo locally . i keep getting the following error after i run cmake .. command :

CMake Warning:
Ignoring extra path from command line:
".."
CMake Error: The source directory "C:/Users/vighn" does not appear to contain CMakeLists.txt.

@vighnesh-kadam Looks like you are invoking CMake command in the wrong folder. In geneal, you want to create a new folder in cuCollections (let's say cuCollections/build). Then inside build, type cmake ../ in the command line. You may want to check CMake tutorial (https://cmake.org/cmake/help/latest/guide/tutorial/index.html) also to understand how it works. It seems you are building in Windows as well and I'm not 100% sure that cuCollections can be built flawlessly in windows. Yours to discover...

@vighnesh-kadam
Copy link
Author

ok i will try the build again in ubuntu

@vighnesh-kadam
Copy link
Author

I am not able to build the repo locally . i keep getting the following error after i run cmake .. command :

CMake Warning:
Ignoring extra path from command line:
".."
CMake Error: The source directory "C:/Users/vighn" does not appear to contain CMakeLists.txt.

@vighnesh-kadam Looks like you are invoking CMake command in the wrong folder. In geneal, you want to create a new folder in cuCollections (let's say cuCollections/build). Then inside build, type cmake ../ in the command line. You may want to check CMake tutorial (https://cmake.org/cmake/help/latest/guide/tutorial/index.html) also to understand how it works. It seems you are building in Windows as well and I'm not 100% sure that cuCollections can be built flawlessly in windows. Yours to discover...

I was able to install cmake on my ubuntu sys. but the problem is im not able to install cuda toolkit hence im not able to get the nvcc compiler , i tried insalling deb and local run from here toolkit install but failed .
im still trying other methods ,
Also it is because my system doesn't have a GPU?

@PointKernel
Copy link
Member

I am not able to build the repo locally . i keep getting the following error after i run cmake .. command :

CMake Warning:
Ignoring extra path from command line:
".."
CMake Error: The source directory "C:/Users/vighn" does not appear to contain CMakeLists.txt.

@vighnesh-kadam Looks like you are invoking CMake command in the wrong folder. In geneal, you want to create a new folder in cuCollections (let's say cuCollections/build). Then inside build, type cmake ../ in the command line. You may want to check CMake tutorial (https://cmake.org/cmake/help/latest/guide/tutorial/index.html) also to understand how it works. It seems you are building in Windows as well and I'm not 100% sure that cuCollections can be built flawlessly in windows. Yours to discover...

I was able to install cmake on my ubuntu sys. but the problem is im not able to install cuda toolkit hence im not able to get the nvcc compiler , i tried insalling deb and local run from here toolkit install but failed . im still trying other methods , Also it is because my system doesn't have a GPU?

Have you tried the apt install? i.e.

sudo apt update
sudo apt install nvidia-cuda-toolkit

Otherwise, you can use a docker image to just compile the code. e.g. https://hub.docker.com/r/nvidia/cuda

@vighnesh-kadam
Copy link
Author

I am not able to build the repo locally . i keep getting the following error after i run cmake .. command :

CMake Warning:
Ignoring extra path from command line:
".."
CMake Error: The source directory "C:/Users/vighn" does not appear to contain CMakeLists.txt.

@vighnesh-kadam Looks like you are invoking CMake command in the wrong folder. In geneal, you want to create a new folder in cuCollections (let's say cuCollections/build). Then inside build, type cmake ../ in the command line. You may want to check CMake tutorial (https://cmake.org/cmake/help/latest/guide/tutorial/index.html) also to understand how it works. It seems you are building in Windows as well and I'm not 100% sure that cuCollections can be built flawlessly in windows. Yours to discover...

I was able to install cmake on my ubuntu sys. but the problem is im not able to install cuda toolkit hence im not able to get the nvcc compiler , i tried insalling deb and local run from here toolkit install but failed . im still trying other methods , Also it is because my system doesn't have a GPU?

Have you tried the apt install? i.e.

sudo apt update
sudo apt install nvidia-cuda-toolkit

Otherwise, you can use a docker image to just compile the code. e.g. https://hub.docker.com/r/nvidia/cuda

yes i tried the sudo apt commands but still got many errors
I will do it using docker now

Comment on lines 35 to 37
auto const num_keys = state.int64("NumInputs");
auto const occupancy = state.float64("Occupancy");
auto const matching_rate = state.float64("MatchingRate");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto const num_keys = state.int64("NumInputs");
auto const occupancy = state.float64("Occupancy");
auto const matching_rate = state.float64("MatchingRate");
auto const num_keys = state.get_int64("NumInputs");
auto const occupancy = state.get_float64("Occupancy");
auto const matching_rate = state.get_float64("MatchingRate");

@sleeepyjack
Copy link
Collaborator

Closing this issue as it's stale and will be implicitly fixed by #110

@sleeepyjack sleeepyjack closed this Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Awaiting reviews before merging type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of get_ prefixes
6 participants