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

Make operand data type and rank validation table-driven #657

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

inexorabletash
Copy link
Contributor

@inexorabletash inexorabletash commented Apr 26, 2024

Improve readability of input operand data type and rank by introducing a table within each method definition that lines the restrictions for positional arguments and options. Steps are updated to reference the table columns.


Preview | Diff

Improve readability of input operand data type and rank by introducing
a table within each method definition that outlines the restrictions
for positional arguments and options. Steps are updated to reference
the table columns.
@inexorabletash inexorabletash changed the title Data type table Make operand data type and rank validation table-driven Apr 27, 2024
@zolkis
Copy link
Collaborator

zolkis commented Apr 29, 2024

I'd prefer this style than the one in #646

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Apr 30, 2024

A few questions to consider:

dataType

  • For most "secondary" operands, the type is constrained to be the same as the primary operand. The relevant steps could be written as either:
    • "If mean’s dataType is not one of its allowed data types, then throw a TypeError." - more consistent.
    • "If mean’s dataType is not the same as input's dataType, then throw a TypeError." - clearer.

rank

  • In most cases there's only one allowed rank. So do we prefer:
    • "If mean’s rank is not its allowed rank, then throw a TypeError." - more consistent.
    • "If mean’s rank is not 1, then throw a TypeError." - clearer.
  • If we prefer the former (reference the table), what about Simplify, correct, and add validation for GRU/LSTM and friends #659 which proposes simplifying some validation e.g. "If mean’s shape is not equal to « input’s shape[options.axis] »..." — this implicitly validates the rank. If we had that, would we still want a step reading "If mean’s rank is not ..." ?
  • If any rank is allowed, do we still want to include the step, for consistency?

General

  • Should we add a "validate the dataType and rank of an operand" helper algorithm? The steps for each operands could then be collapsed to just: "If validating the dataType and rank of mean returns false, then throw a TypeError." - less text, but more is hidden behind the algorithm.
  • When there are multiple input operands, do we prefer separate steps or combined steps? At an extreme we could even write: "If validating the dataType and rank of any of mean, variance, options.scale (if it exists) or options.bias (if it exists) returns false, then throw a TypeError."

@zolkis
Copy link
Collaborator

zolkis commented May 2, 2024

IMHO the "consistent" option is clear enough in this case. :)

- gemm(): Fix ranks in table, align phrasing.
- gru(): Align phrasing.
- lstm(): Don't inline rank of 3, reference table.
- matmul(): Align phrasing.
- prelu(): Fix punctation.
- triangular(): Add table, use for rank validation.
- where(): Align phrasing.
@inexorabletash
Copy link
Contributor Author

Other notes:

  • For the op groups (element-wise binary/unary/logical, pooling, reduction) a table is not given and the inline steps are retained. Any better suggestions?
  • Do we keep a rank validation step if there's a subsequent shape validation step? i.e. keep both of these lines:
    1. If mean’s rank is not its allowed rank, then throw a TypeError.
    2. If mean’s shape is not equal to « input’s shape[options.axis] », then throw a TypeError.

@zolkis
Copy link
Collaborator

zolkis commented May 3, 2024

For the op groups (element-wise binary/unary/logical, pooling, reduction) a table is not given and the inline steps are retained.

Looking at the preview, I think that makes sense.

Do we keep a rank validation step if there's a subsequent shape validation step?

Shape should include rank... I'd leave rank out and let impl optionally handle that as a quick check, if it makes sense somewhere.

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

Successfully merging this pull request may close these issues.

None yet

2 participants