-
Notifications
You must be signed in to change notification settings - Fork 366
Initialization of orthogonal tensors with respect to a pivot #931
base: master
Are you sure you want to change the base?
Initialization of orthogonal tensors with respect to a pivot #931
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I have signed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR! Sorry for the delay, I left some comments for you to fix!
| if ((np.dtype(dtype) is np.dtype(np.complex128)) or | ||
| (np.dtype(dtype) is np.dtype(np.complex64))): | ||
| q,r= decompositions.qr(np,np.random.randn( | ||
| *shape).astype(dtype) + 1j * np.random.randn(*shape).astype(dtype),pivot_axis,non_negative_diagonal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is an else clause missing, otherwise line 804 gets overwritten
| for dtype in dtypes[backend]["rand"]: | ||
| tnI = tensornetwork.initialize_orthogonal_tensor_wrt_pivot( | ||
| shape, | ||
| dtype=dtype,pivot_axis, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that line should throw a syntax error because your passing an argument between named arguments
| shape, | ||
| dtype=dtype,pivot_axis, | ||
| seed=seed, | ||
| backend=backend,non_negative_diagonal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| dtype=dtype,pivot_axis, | ||
| seed=seed, | ||
| backend=backend,non_negative_diagonal) | ||
| npI = backend_obj.initialize_orthogonal_tensor_wrt_pivot(shape, dtype=dtype, pivot_axis, seed=seed,non_negative_diagonal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the function from the backend
| def test_initialize_orthogonal_tensor_wrt_pivot(backend): | ||
| shape=(5, 10, 3, 2) | ||
| pivot_axis=1 | ||
| seed = int(time.time()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use deterministic seed initialization
| return the_tensor | ||
| def initialize_orthogonal_tensor_wrt_pivot(shape=Sequence[int],dtype:Optional[Type[np.number]]=None,pivot_axis:int=-1,seed=Optional[int]=None,backend: Optional[Union[Text, AbstractBackend]] = None,non_negative_diagonal:bool=False) ->Tensor: | ||
| the_tensor=initialize_tensor("randn",shape,backend=backend,seed=seed,dtype=dtype) | ||
| q,r=linalg.qr(the_tensor,pivot_axis,non_negative_diagonal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
us _ instead of r (unused variable)
| the_tensor = initialize_tensor("random_uniform", shape, backend=backend, | ||
| seed=seed, boundaries=boundaries, dtype=dtype) | ||
| return the_tensor | ||
| def initialize_orthogonal_tensor_wrt_pivot(shape=Sequence[int],dtype:Optional[Type[np.number]]=None,pivot_axis:int=-1,seed=Optional[int]=None,backend: Optional[Union[Text, AbstractBackend]] = None,non_negative_diagonal:bool=False) ->Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we could find a less clunky name. Some possibilities that come to my mind are random_orthogonal or random_isometry @alewis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls add a docstring that explains what the function is doing, what the arguments are, and what the returned values are.
| Returns: | ||
| float: Machine epsilon. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this function to the backend? I don't think we need it here
| float: Machine epsilon. | ||
| """ | ||
| return np.finfo(dtype).eps | ||
| def initialize_orthogonal_tensor_wrt_pivot(self,shape=Sequence[int],dtype:Optional[Type[np.number]]=None,pivot_axis:int=-1,seed=Optional[int]=None,backend: Optional[Union[Text, AbstractBackend]] = None,non_negative_diagonal: bool = False):->Tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this function
| seed=seed, | ||
| backend=backend,non_negative_diagonal) | ||
| npI = backend_obj.initialize_orthogonal_tensor_wrt_pivot(shape, dtype=dtype, pivot_axis, seed=seed,non_negative_diagonal) | ||
| np.testing.assert_allclose(tnI.array, npI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls replace with a test that checks if the initialized tensor has the desired properties
created a new method in tensornetwork/linalg/initialization.py for initializing a random tensor with entries distributed according to normal distribution and performing QR Decomposition on it and returning the tensor Q so that when a tensor is contracted about a given pivot index,the result is orthogonal