add cross #850

mtar · 2021-07-29T09:23:32Z

Description

implementation of the cross product for 2d and 3d vectors

Issue/s resolved: #844

Changes proposed:

add cross

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 · 2021-07-29T09:24:09Z

Codecov Report

Merging #850 (88038d1) into master (bc74848) will increase coverage by 0.01%.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
+ Coverage   95.49%   95.50%   +0.01%     
==========================================
  Files          64       64              
  Lines        9535     9579      +44     
==========================================
+ Hits         9105     9148      +43     
- Misses        430      431       +1

Flag	Coverage Δ
gpu	`94.63% <97.72%> (+0.01%)`	⬆️
unit	`91.06% <95.45%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
heat/core/linalg/basics.py	`95.04% <97.72%> (+0.15%)`	⬆️

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 bc74848...88038d1. Read the comment docs.

coquelin77 · 2021-08-02T14:18:39Z

heat/core/linalg/basics.py

+    if x1.split != x2.split:
+        raise ValueError(
+            "'x1' and 'x2' must have the same split, {} != {}".format(x1.split, x2.split)
+        )


is there an open issue for this? we should probably have this functionality

could we use halos to do this?

coquelin77 · 2021-08-02T14:23:18Z

heat/core/linalg/basics.py

+    if x1.comm != x2.comm:  # pragma: no cover
+        raise ValueError("'x1' and 'x2' must have the same comm, {} != {}".format(x1.comm, x2.comm))


this may result in errors. in most places we assume that all DNDarrays are mapped across all processes the same way. I am very uncertain if this will cause errors at scale.

coquelin77 · 2021-08-02T14:28:12Z

heat/core/linalg/basics.py

+
+    promoted = torch.promote_types(x1.larray.dtype, x2.larray.dtype)
+
+    ret = torch.cross(x1.larray.type(promoted), x2.larray.type(promoted), dim=axis)


so if im reading the correctly, this is a limited cross product function. do you have plans to implement more cases?

What other cases? The only restriction I made deliberately was not allowing the split axis of the DNDarray as the axis argument. What do you miss?

it actually doesn't make any sense to split along the vector axis, it can only be 2 or 3 elements. What's missing is being able to set the axisa, axisb parameters, and being able to perform cross products of 2D with 3D vectors.

coquelin77 · 2021-08-02T14:38:35Z

oh also, i think it would be useful to add correctness tests to the unittests

ClaudiaComito

@mtar thank you so much for addressing this, it's great to have it! I have some changes requests, mainly to do with the numpy-API compliance.

heat/core/linalg/basics.py

ClaudiaComito · 2021-09-13T15:03:07Z

heat/core/linalg/basics.py

+    sanitation.sanitize_in(x1)
+    sanitation.sanitize_in(x2)
+
+    if x1.gshape != x2.gshape:


shapes should be the same except for the axes that define the vectors. Example:

import numpy as np a = np.arange(2*12).reshape(2,-1,3) b = np.arange(2*4*2).reshape(2,4,2) np.cross(a,b) array([[[ -2, 0, 0], [ -15, 10, 1], [ -40, 32, 2], [ -77, 66, 3]], [[-126, 112, 4], [-187, 170, 5], [-260, 240, 6], [-345, 322, 7]]]) # different vector axis for b b = np.transpose(b, (0, 2, 1)) b.shape (2, 2, 4) a.shape (2, 4, 3) np.cross(a, b, axisb=1) array([[[ -2, 0, 0], [ -15, 10, 1], [ -40, 32, 2], [ -77, 66, 3]], [[-126, 112, 4], [-187, 170, 5], [-260, 240, 6], [-345, 322, 7]]])

ClaudiaComito · 2021-09-13T15:03:39Z

heat/core/linalg/basics.py

+
+    if x1.gshape != x2.gshape:
+        raise ValueError(
+            "'x1' and 'x2' must have the same shape, {} != {}".format(x1.gshape, x2.gshape)


I think the check here should be whether a.shape and b.shape are broadcastable after purging axisa, axisb

ClaudiaComito · 2021-09-13T15:12:57Z

heat/core/linalg/basics.py

+        )
+    if x1.split != x2.split:
+        raise ValueError(
+            "'x1' and 'x2' must have the same split, {} != {}".format(x1.split, x2.split)


the splits must match after purging the vector axis

i dont know what you mean here. Is this error message not sufficient?

ClaudiaComito · 2021-09-13T15:15:19Z

heat/core/linalg/basics.py

+        )
+    if x1.device != x2.device:
+        raise ValueError(
+            "'x1' and 'x2' must have the same device type, {} != {}".format(x1.device, x2.device)


why? one of them will be copied to cpu, but the operation can still be performed, right?

I can remove the check. PyTorch will throw an error about the device mismatch then.

ClaudiaComito · 2021-09-13T15:16:53Z

heat/core/linalg/basics.py

+        x1.balance_()
+        x2.balance_()


ClaudiaComito · 2021-09-13T15:18:29Z

heat/core/linalg/basics.py

+
+    promoted = torch.promote_types(x1.larray.dtype, x2.larray.dtype)
+
+    ret = torch.cross(x1.larray.type(promoted), x2.larray.type(promoted), dim=axis)


it actually doesn't make any sense to split along the vector axis, it can only be 2 or 3 elements. What's missing is being able to set the axisa, axisb parameters, and being able to perform cross products of 2D with 3D vectors.

ClaudiaComito · 2021-09-13T15:24:39Z

heat/core/linalg/tests/test_basics.py

+        self.assertEqual(cross.comm, a.comm)
+        self.assertEqual(cross.device, a.device)
+        self.assertTrue(ht.equal(cross, ht.array([[0, 0, -1], [-1, 0, 0], [0, -1, 0]])))
+


add tests for axisa, axisb, 2D with 3D cross product

mtar · 2021-10-05T08:09:27Z

@mtar thank you so much for addressing this, it's great to have it! I have some changes requests, mainly to do with the numpy-API compliance.

I was following the Python array API standard on that one skipping NumPy's quirks 😃
https://data-apis.org/array-api/latest/extensions/linear_algebra_functions.html#linalg-cross-x1-x2-axis-1

coquelin77 · 2021-11-09T09:45:25Z

are all of the comments addressed here?

…at into feature/844-cross

Implement axisa, axisb, axisc functionality for ht.cross

ClaudiaComito

Thanks @mtar !

mtar added 2 commits July 29, 2021 11:20

add cross

4803de7

update docstring

fac0e50

add more tests

3e0627c

mtar requested a review from coquelin77 July 29, 2021 09:48

update type hints

b339bc3

coquelin77 reviewed Aug 2, 2021

View reviewed changes

ClaudiaComito added this to the 1.2.x milestone Aug 18, 2021

ClaudiaComito requested changes Sep 13, 2021

View reviewed changes

mtar and others added 4 commits October 6, 2021 15:14

Merge branch 'master' into feature/844-cross

dc3c0ec

Merge branch 'master' into feature/844-cross

87395e0

Merge branch 'master' into feature/844-cross

2db614f

linalg.basics - inserted deleted matrix norm in all

b8be839

coquelin77 previously approved these changes Oct 8, 2021

View reviewed changes

Merge branch 'master' into feature/844-cross

6a9b7fc

mtar added 2 commits December 10, 2021 15:46

Merge branch 'master' into feature/844-cross

c043eac

Merge branch 'feature/844-cross' of github.com:helmholtz-analytics/he…

1d1434d

…at into feature/844-cross

mtar added ❗ High priority, urgent and removed ❗ High priority, urgent labels Dec 10, 2021

Allow 2d vectors

8ddef62

mtar dismissed coquelin77’s stale review via 8ddef62 December 13, 2021 16:23

ClaudiaComito and others added 2 commits December 21, 2021 11:49

Feature/844 cross axisa axisb (#899)

fa643da

Implement axisa, axisb, axisc functionality for ht.cross

Merge branch 'master' into feature/844-cross

88038d1

ClaudiaComito approved these changes Jan 11, 2022

View reviewed changes

ClaudiaComito merged commit 293d873 into master Jan 11, 2022

ClaudiaComito deleted the feature/844-cross branch January 11, 2022 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cross #850

add cross #850

mtar commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

coquelin77 Aug 2, 2021

coquelin77 Aug 2, 2021

coquelin77 Aug 2, 2021

coquelin77 Aug 2, 2021

mtar Aug 27, 2021 •

edited

Loading

ClaudiaComito Sep 13, 2021

coquelin77 commented Aug 2, 2021

ClaudiaComito left a comment

ClaudiaComito Sep 13, 2021

ClaudiaComito Sep 13, 2021

ClaudiaComito Sep 13, 2021

coquelin77 Oct 8, 2021

ClaudiaComito Sep 13, 2021

mtar Oct 5, 2021

ClaudiaComito Sep 13, 2021

ClaudiaComito Sep 13, 2021

ClaudiaComito Sep 13, 2021

mtar commented Oct 5, 2021

coquelin77 commented Nov 9, 2021

ClaudiaComito left a comment

		if x1.comm != x2.comm: # pragma: no cover
		raise ValueError("'x1' and 'x2' must have the same comm, {} != {}".format(x1.comm, x2.comm))


		promoted = torch.promote_types(x1.larray.dtype, x2.larray.dtype)

		ret = torch.cross(x1.larray.type(promoted), x2.larray.type(promoted), dim=axis)

add cross #850

add cross #850

Conversation

mtar commented Jul 29, 2021 • edited Loading

Description

Changes proposed:

Type of change

Due Diligence

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

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtar Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coquelin77 commented Aug 2, 2021

ClaudiaComito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtar commented Oct 5, 2021

coquelin77 commented Nov 9, 2021

ClaudiaComito left a comment

Choose a reason for hiding this comment

mtar commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

mtar Aug 27, 2021 •

edited

Loading