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

Binary special methods return NotImplemented #1191

Merged

Conversation

mtar
Copy link
Collaborator

@mtar mtar commented Aug 4, 2023

Due Diligence

  • General:
    • base branch must be main for new features, latest release branch (e.g. release/1.3.x) for bug fixes
    • title of the PR is suitable to appear in the Release Notes
  • Implementation:
    • unit tests: all split configurations tested
    • unit tests: multiple dtypes tested
    • documentation updated where needed

Description

This Pull Request adds special functions for binary arithmetic operations to DNDarray, such that they return NotImplemented on unsupported inputs.

Issue/s resolved: #311, #958 (partially)

Changes proposed:

Type of change

Enhancement

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

yes / no

@ghost
Copy link

ghost commented Aug 4, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Thank you for the PR!

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #1191 (9d7c987) into main (d02df3a) will increase coverage by 0.08%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
+ Coverage   91.75%   91.83%   +0.08%     
==========================================
  Files          77       77              
  Lines       11080    11191     +111     
==========================================
+ Hits        10166    10277     +111     
  Misses        914      914              
Flag Coverage Δ
unit 91.83% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
heat/core/_operations.py 96.11% <ø> (ø)
heat/core/arithmetics.py 99.71% <100.00%> (+0.11%) ⬆️
heat/core/linalg/basics.py 95.55% <100.00%> (+0.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@mtar mtar changed the title return NotImplemented on DNDarray methods Binary special methods return NotImplemented Sep 18, 2023
@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@mtar mtar marked this pull request as ready for review September 18, 2023 14:39
@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

mrfh92
mrfh92 previously approved these changes Oct 30, 2023
Copy link
Collaborator

@mrfh92 mrfh92 left a comment

Choose a reason for hiding this comment

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

fine 👍

heat/core/arithmetics.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

Copy link
Collaborator

@mrfh92 mrfh92 left a comment

Choose a reason for hiding this comment

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

Last changes in the docs look fine. I recommend merging if the test run through.
Thx for the work 👍

@mtar mtar requested review from ClaudiaComito and removed request for ClaudiaComito October 31, 2023 09:30
@ClaudiaComito ClaudiaComito added this to the 1.4.0 milestone Nov 1, 2023
Copy link
Contributor

github-actions bot commented Nov 2, 2023

Thank you for the PR!

@mtar mtar dismissed ClaudiaComito’s stale review November 2, 2023 09:58

requested changes added, old review blocks merging

@mtar mtar merged commit f62e2b1 into main Nov 2, 2023
9 checks passed
@mtar mtar deleted the enhancement/311-binary-arithmetic-operations-notImplemented branch November 2, 2023 10:20
@mtar mtar removed the PR talk label Nov 6, 2023
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.

Binary operators should return NotImplemented
3 participants