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

Feature arctan2 #593

Merged
merged 11 commits into from
Jun 16, 2020
Merged

Feature arctan2 #593

merged 11 commits into from
Jun 16, 2020

Conversation

mtar
Copy link
Collaborator

@mtar mtar commented Jun 8, 2020

Description

Issue/s resolved: #546

This PR adds a arctan2 function to HeAT

Changes proposed:

  • arctan2 function

Type of change

New feature

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

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

no

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #593 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   96.48%   96.49%           
=======================================
  Files          75       75           
  Lines       15214    15249   +35     
=======================================
+ Hits        14679    14714   +35     
  Misses        535      535           
Impacted Files Coverage Δ
heat/core/tests/test_trigonometrics.py 98.55% <100.00%> (+0.13%) ⬆️
heat/core/trigonometrics.py 95.45% <100.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9108523...8a1e45c. Read the comment docs.

@mtar mtar marked this pull request as ready for review June 9, 2020 07:09
>>> ht.arctan2(y, x) * 180 / ht.pi
tensor([-135.0000, -45.0000, 45.0000, 135.0000], dtype=torch.float64)
"""
# Special Case: integer -> float because torch.atan2() does not work with integer types on version 1.5.0.
Copy link
Member

Choose a reason for hiding this comment

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

This should probably issued as warning to the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. numpy allows integer types. I rewrote the comment a little bit.

@@ -99,6 +102,44 @@ def arctan(x, out=None):
return local_op(torch.atan, x, out)


def arctan2(x1, x2, out=None):
Copy link
Member

Choose a reason for hiding this comment

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

Charlie updated our docstrings (to be found in branch 504). This function needs

a) a return type annotation
b) DNDarray should be :class:DNDarray

Also out should be either removed if not used/applicable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope I found everything.

@mtar mtar removed the request for review from Cdebus June 10, 2020 14:32
Copy link
Contributor

@Cdebus Cdebus left a comment

Choose a reason for hiding this comment

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

Sorry, have to be picky in the beginning about the new docstring formats... will add comments for now to every pull request to explain the style

import torch
from .constants import pi
from .operations import __local_op as local_op
from .operations import __binary_op as binary_op
from . import dndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be from .dndarray import DNDarray, then DNDarray can be annotated directly

@@ -99,6 +105,32 @@ def arctan(x, out=None):
return local_op(torch.atan, x, out)


def arctan2(x1, x2) -> dndarray.DNDarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

just -> DNDarray


Parameters
----------
x1 : :class:DNDarray
Copy link
Contributor

Choose a reason for hiding this comment

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

here, the :class:is not needed, can be just DNDarray

def arctan2(x1, x2) -> dndarray.DNDarray:
"""
Element-wise arc tangent of ``x1/x2`` choosing the quadrant correctly.
Returns a new :class:``DNDarray`` with the signed angles in radians between vector (``x2``,``x1``) and vector (1,0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok, DNDarray will now be displayed in markdown code-style. If you want to create a cross-reference link to the DNDarray class, you must use :class:~heat.core.dndarray.DNDarray .... with the ~ in front the whole module path will not be displayed in the docs. However, since the returntype is already a link, I don't think this is necessary here, just a note for the future :)

----------
x1 : :class:DNDarray
y-coordinates
x2 : :class:DNDarray
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, :class: not needed

x1 : :class:DNDarray
y-coordinates
x2 : :class:DNDarray
x-coordinates. If ``x1.shape != x2.shape``, they must be broadcastable to a common shape (which becomes the shape of the output).
Copy link
Contributor

Choose a reason for hiding this comment

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

in Markdown code style, please refrain from using whitespaces around operators (it looks shitty in the docs due to the different font and size)

x2 : :class:DNDarray
x-coordinates. If ``x1.shape != x2.shape``, they must be broadcastable to a common shape (which becomes the shape of the output).

Example
Copy link
Contributor

Choose a reason for hiding this comment

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

Examples

Copy link
Contributor

@Cdebus Cdebus left a comment

Choose a reason for hiding this comment

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

I'm ok with this :)

@Markus-Goetz Markus-Goetz merged commit 547d33f into master Jun 16, 2020
@Markus-Goetz Markus-Goetz deleted the features/546-arctan2 branch June 16, 2020 11:07
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.

Implement arctan2()
3 participants