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

API Refactoring to export symbols in submodules explicitly #51

Merged
merged 7 commits into from
Oct 23, 2024

Conversation

youwuyou
Copy link
Collaborator

@youwuyou youwuyou commented Oct 8, 2024

As addressed in issue #44 , it would be desirable to refactor the current API and ship it with the next release of Chmy such that users can simply use using Chmy instead of concrete import of submodules as we have now. This PR starts with a state where all submodules have been refactored besides two, which we will adress below.

With the aim to avoid the usage of additional Reexport.jl package, we followed a similar approach as used in Oceananigans.jl.

The remaining things to do include the followings, I would like to discuss with @utkinis and @luraess and see how we want to best do it before further refactoring:

  • Explicitly export Grids and GridOperators, ➡️ accordingly remove usage of using Grids and using GridOperators in the test suite
  • get_backend() of Architectures module name clashes with KernelAbstractions.jl ➡️ Thus is test suite the module is used explicitly yet

@youwuyou youwuyou added the enhancement New feature or request label Oct 8, 2024
@youwuyou youwuyou requested review from utkinis and luraess October 8, 2024 15:20
@youwuyou youwuyou self-assigned this Oct 8, 2024
@youwuyou
Copy link
Collaborator Author

youwuyou commented Oct 8, 2024

To the first item: the Grids and GridOperators submodules contain parts where the symbols are exported in a rather exotics way (ref. cartesian_field_operators.jl for an example), how do we want to reexport these expressions?

@youwuyou
Copy link
Collaborator Author

youwuyou commented Oct 8, 2024

To the second item, currently users need to explicitly use KernelAbstractions.jl (and I think it is good this way). But possible name clashes could be problematic, do we want to rename the relevant functions or is there maybe a workaround for it too?

@youwuyou
Copy link
Collaborator Author

youwuyou commented Oct 8, 2024

Some clean-up jobs would be to modify relevant parts of the documentation & the examples (this is not necessary, because using submodules works too even with our API change, but maybe good for users to avoid confusions?).

@utkinis
Copy link
Member

utkinis commented Oct 8, 2024

To the first item: the Grids and GridOperators submodules contain parts where the symbols are exported in a rather exotics way (ref. cartesian_field_operators.jl for an example), how do we want to reexport these expressions?

I suggest doing it in the most straightforward way. These "exotic" exports are only needed to generate functions for Cartesian grids with coordinate names x, y, z, which will probably change in the future, but not from API perspective. So exports from Chmy.Grids and Chmy.GridOperators will contain following lines:

# Grids
export coord, xcoord, ycoord, zcoord
export Δ, Δx, Δy, Δz
# other exports...

# GridOperators
export δ, δx, δy, δz
export ∂, ∂x, ∂y, ∂z
# other exports...

@utkinis
Copy link
Member

utkinis commented Oct 8, 2024

To the second item, currently users need to explicitly use KernelAbstractions.jl (and I think it is good this way). But possible name clashes could be problematic, do we want to rename the relevant functions or is there maybe a workaround for it too?

Yeah, I should've done this long time ago, here it should be:

KernelAbstractions.get_backend(arch::SingleDeviceArchitecture) = arch.backend

which would remove the name clash

@luraess
Copy link
Member

luraess commented Oct 16, 2024

Are we good to go with this one @utkinis ?

@utkinis
Copy link
Member

utkinis commented Oct 22, 2024

Thanks for the work @youwuyou and @luraess ! Looks good to me, I guess it deserves a non-breaking release

@utkinis utkinis merged commit 8ecd0b5 into main Oct 23, 2024
9 checks passed
@utkinis utkinis deleted the yw/export-refactoring branch October 23, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants