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

Capitalization in EnzymeCore #2164

Open
gdalle opened this issue Dec 4, 2024 · 0 comments
Open

Capitalization in EnzymeCore #2164

gdalle opened this issue Dec 4, 2024 · 0 comments

Comments

@gdalle
Copy link
Contributor

gdalle commented Dec 4, 2024

Warning

The following propositions are very breaking and not essential, so weigh them carefully.

Since a breaking release may be on the horizon, here are some of my gripes about capitalization and style. My reference is the official (albeit concise) Julia style guide.

Mode setters

Use naming conventions consistent with Julia base/:

  • modules and type names use capitalization and camel case: module SparseArrays, struct UnitRange.
  • functions are lowercase (maximum, convert) and, when readable, with multiple words squashed together (isequal, haskey). When necessary, use underscores as word separators.

Mode setters are functions. At the moment, some are already lowercase

clear_err_if_func_written
clear_runtime_activity
set_abi
set_err_if_func_written
set_runtime_activity

but some are uppercase and could be renamed (possibly while keeping the deprecated version around with a warning):

Combined => combined_mode
NoPrimal => noprimal_mode
Split => split_mode  # split already exists
WithPrimal => with_primal

Mode objects

Avoid confusion about whether something is an instance or a type

To me, the following choices in Enzyme seemed rather confusing at first:

julia> ForwardMode isa Type
true

julia> Forward isa Type
false

julia> Forward isa ForwardMode
true

If renaming were easy, I would suggest either of the following conventions instead:

  1. make mode instances all-uppercase like other module-level constants (recommendation found in BlueStyle and SciMLStyle
Forward => FORWARD
ForwardWithPrimal => FORWARD_WITH_PRIMAL
Reverse => REVERSE
ReverseWithPrimal => REVERSE_WITH_PRIMAL
ReverseHolomorphic => REVERSE_HOLOMORPHIC
ReverseHolomorphicWithPrimal => REVERSE_HOLOMORPHIC_WITH_PRIMAL
ReverseSplitNoPrimal => REVERSE_SPLIT_NO_PRIMAL
ReverseSplitWithPrimal => REVERSE_SPLIT_WITH_PRIMAL
  1. obtain mode instances as the output of constructors, i.e. add parentheses
Forward => Forward()
ForwardWithPrimal => ForwardWithPrimal()
Reverse => Reverse()
ReverseWithPrimal => ReverseWithPrimal()
ReverseHolomorphic => ReverseHolomorphic()
ReverseHolomorphicWithPrimal => ReverseHolomorphicWithPrimal()
ReverseSplitNoPrimal => ReverseSplitNoPrimal()
ReverseSplitWithPrimal => ReverseSplitWithPrimal()

However I realize that this would break everyone's code everywhere, so this is obviously low priority unless there is community consensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant