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

Rename functions starting from under score #115

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Jul 6, 2024

Improves #81

Following functions have been renamed

_get_shift => get_shift
_roll => roll_impl
_fftshift => fftshift_impl
_ifftshift => ifftshift_impl
_normalize => normalize_impl
_coefficients => coefficients_impl
_crop_or_pad => crop_or_pad_impl
_prep_transpose_view => prep_transpose_view
_transpose => transpose_impl
_create => create_plan
_init_threads => init_threads
_exec => exec_plan
_destroy_plan, _destroy_info => destroy_plan_and_info

After review, following changes are made.

_roll => roll
_coefficients => get_coefficients
_destroy_plan => destroy_plan
_destroy_info => destroy_info

@yasahi-hpc yasahi-hpc self-assigned this Jul 6, 2024
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

I would suggest not to use _impl names in the Impl namespace.

common/src/KokkosFFT_Helpers.hpp Outdated Show resolved Hide resolved
fft/src/KokkosFFT_Host_plans.hpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

I would suggest not to use _impl names in the Impl namespace.

Thank you for your review.
I have renamed _roll and _coefficients functions.

Some functions like normalize_impl are unchanged because normalize is the name of (internal) API and it conflicts.
Would it be better to rename normalize_impl to normalize_exec, for example?

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

I prefer _impl than _exec.

@yasahi-hpc
Copy link
Collaborator Author

I prefer _impl than _exec.

OK. I will merge this. Thank you for your review.

@yasahi-hpc yasahi-hpc merged commit 17e97f3 into kokkos:main Jul 9, 2024
19 checks passed
@yasahi-hpc yasahi-hpc deleted the rename-functions branch July 9, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants