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

Remove the compose operator from the fn API table #3767

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Mar 29, 2022

  • as the compose operator is not supported in the functional
    API this PR removes it from the supported OP table

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Category:

Other (e.g. Documentation, Tests, Configuration)

Description:

  • as the compose operator is not supported in the functional
    API this PR removes it from the supported OP table

Additional information:

Affected modules and functionalities:

  • operators table

Key points relevant for the review:

  • look and feel

Checklist

Tests

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

- as the compose operator is not supported in the functional
  API this PR removes it from the supported OP table

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Copy link
Contributor Author

JanuszL commented Mar 29, 2022

image

Comment on lines 88 to 89
if op_name.lower() in removed_ops:
fn_string = "NA"
Copy link
Contributor

@mzient mzient Mar 29, 2022

Choose a reason for hiding this comment

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

Why .lower()? I think it should use either ops style OperatorName or fn style operator_name, not yet another variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend creating a function def has_fn_variant(op_name) or similar

def has_fn_variant(op_name):
    no_fn_list = ['Compose']
    return False if op_name in no_fn_list else True   

then use it here and in line 106

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -80,6 +85,8 @@ def fn_to_op_table(out_filename):
if m is not None and hasattr(m, op_name):
op_string = link_formatter.format(op = op_full_name, module = module_name)
fn_string = link_formatter.format(op = to_fn_name(op_full_name), module = to_fn_module(module_name))
if op_name.lower() in removed_ops:
fn_string = "NA"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Maybe "Not Applicable" or "N/A".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Copy link
Contributor Author

JanuszL commented Mar 29, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4263231]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4263231]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [4263231]: BUILD PASSED

@JanuszL
Copy link
Contributor Author

JanuszL commented Mar 29, 2022

The most recent:
image

@JanuszL JanuszL merged commit 9b24277 into NVIDIA:main Mar 29, 2022
@JanuszL JanuszL deleted the remove_compose_from_tab branch March 29, 2022 17:39
cyyever pushed a commit to cyyever/DALI that referenced this pull request May 13, 2022
- as the compose operator is not supported in the functional
  API this PR removes it from the supported OP table

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022
- as the compose operator is not supported in the functional
  API this PR removes it from the supported OP table

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
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

Successfully merging this pull request may close these issues.

4 participants