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

Standardize naming #351

Open
sdasgup3 opened this issue Oct 17, 2022 · 7 comments
Open

Standardize naming #351

sdasgup3 opened this issue Oct 17, 2022 · 7 comments
Labels

Comments

@sdasgup3
Copy link
Member

Request description

The need for such a ticket initiated in #335 (comment).

Let us use this ticket to collect opportunities to apply uniform naming across various StableHLO artifacts like the entities in the ODS.

From #335 (review)

... ensure that: 1) the opset has uniform naming style, 2) all C++/Python breaking changes related to naming are done at once instead of spread over weeks or months of breakages.
@ghpvnist
Copy link
Member

ghpvnist commented Nov 7, 2022

Some ops have inconsistent naming with our standard e.g. using "output" instead of "result" in the ODS. The spec, ODS example, and pretty-print currently defaults to "result" even if the ODS name is "output". These ops are (in no particular order):

  • Constant
  • Iota
  • IsFinite
  • DynamicIota
  • CreateToken
  • BatchNormTraining
  • DynamicBroadcastInDim
  • DynamicReshape
  • RngBitGenerator
  • ReducePrecision

burmako pushed a commit that referenced this issue Nov 7, 2022
Thank you, Sandeep, for discovering and documenting this in #351!
@burmako burmako removed the Migrate to MHLO PR that needs to be migrated to MLIR-HLO label Nov 8, 2022
@burmako burmako changed the title Allow uniform naming across various StableHLO artifacts Decide on uniform naming across various StableHLO artifacts Nov 9, 2022
GleasonK pushed a commit to GleasonK/stablehlo that referenced this issue Nov 10, 2022
Thank you, Sandeep, for discovering and documenting this in openxla#351!
@burmako
Copy link
Contributor

burmako commented Nov 16, 2022

Looks like log_plus_one is a misnomer. Unlike exponential_minus_one, which computes exp(x) - 1, this op computes log(1 + x), so I think that it should be renamed to log_one_plus, consistently with the short name of the op which is log1p (cf. expm1).

@burmako burmako changed the title Decide on uniform naming across various StableHLO artifacts Standardize naming Nov 25, 2022
@burmako
Copy link
Contributor

burmako commented Nov 25, 2022

Some more ideas for standardizing names:

  1. input vs operand (output vs result already called out above).
  2. Consistent use of singulars and plurals (e.g. TupleOp::val should be vals, etc).
  3. IotaOp::iota_dimension (and multiple similar cases) vs ReverseOp::dimensions.
  4. Specifying the "result" name explicitly vs relying on TableGen to provide name it implicitly.
  5. Same thing as above but for derived unary and binary ops.
  6. IsFiniteOp:x/y - can we use standard names?
  7. CompareOp::compare_type - why not comparison_type, given that this is how the enum is called?
  8. CholeskyOp::a - can we use standard name?
  9. DotGeneralOp::dot_dimension_numbers vs ConvolutionOp::dimension_numbers.
  10. CompareOp::comparison_direction/FftOp::fft_type/etc vs just direction/type/etc.
  11. Compare names with HLO and see if that exposes more inconsistencies. Would be good to align with HLO if possible.
  12. Compare op names and op mnemonics. E.g. why SinOp has the "sine" mnemonic? Can we have op names and op mnemonics be the same (modulo case).

burmako pushed a commit that referenced this issue Dec 3, 2022
fixes #525 

verification of infeed op is revisit based on
#639

Note that for outfeed op we are using the output name as `results` based
on the implicit name [introduced by the tablegen
file](https://github.com/openxla/stablehlo/blob/dbf9964c6ce980e1976041700807bd94e6adf849/stablehlo/dialect/StablehloOps.td#L1030
) even though it should be explicitly called out as `result` (singular).
This will be addressed in
#351 (comment),
item 4.
GleasonK pushed a commit to GleasonK/stablehlo that referenced this issue Dec 6, 2022
fixes openxla#525 

verification of infeed op is revisit based on
openxla#639

Note that for outfeed op we are using the output name as `results` based
on the implicit name [introduced by the tablegen
file](https://github.com/openxla/stablehlo/blob/dbf9964c6ce980e1976041700807bd94e6adf849/stablehlo/dialect/StablehloOps.td#L1030
) even though it should be explicitly called out as `result` (singular).
This will be addressed in
openxla#351 (comment),
item 4.
@ghpvnist
Copy link
Member

ghpvnist commented Dec 8, 2022

The mnemonic for CrossReplicaSumOp is cross-replica-sum, which is not consistent with other mnemonics which uses underscore case: cross_replica_sum. This needs to be changed in status.md, StablehloOps.td, and other related files (e.g. including tests/mlir files).

@burmako
Copy link
Contributor

burmako commented Feb 18, 2023

Naming affects serialization, and this changes the project for this ticket from the P1 priority Public API to the P0 priority Specification Compliance. We'll need to sort this out before the release of v1.0.

@ghpvnist
Copy link
Member

Another instance where naming can improve in TupleOp.

@ghpvnist
Copy link
Member

ghpvnist commented Feb 6, 2024

Instances here and here in collective_broadcast where change in attribute names are suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

3 participants