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

Added cases in permutedims for sparse arrays #17035

Merged
merged 5 commits into from Jul 3, 2019

Conversation

Projects
None yet
4 participants
@kangzhiq
Copy link
Contributor

commented Jun 16, 2019

References to other Issues or PRs

#16941 permutedims and transpose

Brief description of what is fixed or changed

Fixed issue about casting sparse array to list in permutedims as well as transpose, since the later one call implicitly permutedims(Array, (1, 0)) for evaluation.
Before:

>>> b = MutableSparseNDimArray.zeros(20000, 10000, 20000, 4000)
>>> permutedims(b, (0, 2))
MemoryError

now:

>>> A = MutableSparseNDimArray.zeros(20000, 10000, 20000, 4000)
>>> permutedims(A, (0, 1, 2)) == A

Other comments

[Don't merge] please.
Some tests would fail if we include sparse array into the test loop.

Release Notes

  • tensor
    • Added specific cases for sparse array in permutedims and tests
Added cases in permutedims for spar_arr
- Added cases in permutedims for sparse arrays
- Added tests accordingly
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 16, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • tensor
    • Added specific cases for sparse array in permutedims and tests (#17035 by @kangzhiq)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
#16941 `permutedims `and `transpose`

#### Brief description of what is fixed or changed
Fixed issue about casting sparse array to list in `permutedims` as well as `transpose`, since the later one call implicitly `permutedims(Array, (1, 0))` for evaluation.
Before:
```python
>>> b = MutableSparseNDimArray.zeros(20000, 10000, 20000, 4000)
>>> permutedims(b, (0, 2))
MemoryError
```
now:
```python
>>> A = MutableSparseNDimArray.zeros(20000, 10000, 20000, 4000)
>>> permutedims(A, (0, 1, 2)) == A
```

#### Other comments
[Don't merge] please.
Some tests would fail if we include sparse array into the test loop.

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* tensor
  * Added specific cases for sparse array in `permutedims `and tests
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@kangzhiq

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

The error on the test:

Elapsed: 00:59.228649
CF-RAY: 4e78e2cb7fb29d8f-ORD
An HTTP error occurred when trying to retrieve this URL.
HTTP errors are often intermittent, and a simple retry will get you on your way.
@codecov

This comment has been minimized.

Copy link

commented Jun 16, 2019

Codecov Report

Merging #17035 into master will increase coverage by 0.096%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #17035       +/-   ##
=============================================
+ Coverage   74.394%   74.491%   +0.096%     
=============================================
  Files          622       623        +1     
  Lines       160776    161348      +572     
  Branches     37738     37868      +130     
=============================================
+ Hits        119608    120190      +582     
+ Misses       35839     35827       -12     
- Partials      5329      5331        +2
index = 0
for i, idx in enumerate(perm(expr._get_tuple_index(k))):
index = index * new_shape[i] + idx
new_array[index] = v

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 16, 2019

Contributor

are you sure you can't do this in one line? I believe you can use a dictionary comprehension.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jun 16, 2019

Author Contributor

I am afraid not. the problem is in the inner loop. The inner loop sums up terms in a succesive way, so I couldn't see how dict comprehension can be used in this case. Even Sum() is not applicable here I think.
In fact the inner loop have the same functionality as _parse_index of NDimArray. One alternative solution would be:

# Initializing a mutable sparse array at first
temp_arr = MutableSparseNDimArray({}, new_shape)
temp_arr._sparse_array = {temp_arr._parse_index(perm(expr._get_tuple_index(k))):v 
                              for k,v in expr._sparse_array.items()}
if isinstance(expr, Immutable):
    return temp_arr.as_immutable()
return temp_arr

But the code is not very readable and it will create an unnecessary new mutable object if expr is immutable
What do you think of it?

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 18, 2019

Contributor

what about:

return type(expr)(
    {tuple(perm(expr._get_tuple_index(k))): v for k, v in expr._sparse_array.items()},
    new_shape
)
new_shape = perm(expr.shape)

if isinstance(expr, SparseNDimArray):
new_array = {}

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 16, 2019

Contributor

call it new_dict, it's not an array.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jun 16, 2019

Author Contributor

I was calling it new_array to respect the existing convention. Some lines below there is a list being called as new_array and then passed to the constructor. I think the code might be more comprehensible if we keep that convention.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 18, 2019

Contributor

Which convention are you talking about? Suppose you were to write the code in C++, how would you call the variables?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 1, 2019

Author Contributor

I was trying to use the same name for the same variable in one function, I thought it could help improve the coherence within the function. Anyway there is no longer this problem.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 2, 2019

Contributor

No, it's confusing if you read the code. You are calling a dictionary an array, that's wrong.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 2, 2019

Author Contributor

Ok, I got your point. I will keep this in mind. :-)

@@ -277,3 +277,11 @@ def test_array_permutedims():
assert po2 == Array([[[[sa[0], sa[1]], [sa[2], sa[3]]], [[sa[4], sa[5]], [sa[6], sa[7]]], [[sa[8], sa[9]], [sa[10], sa[11]]], [[sa[12], sa[13]], [sa[14], sa[15]]], [[sa[16], sa[17]], [sa[18], sa[19]]], [[sa[20], sa[21]], [sa[22], sa[23]]], [[sa[24], sa[25]], [sa[26], sa[27]]], [[sa[28], sa[29]], [sa[30], sa[31]]], [[sa[32], sa[33]], [sa[34], sa[35]]]], [[[sa[36], sa[37]], [sa[38], sa[39]]], [[sa[40], sa[41]], [sa[42], sa[43]]], [[sa[44], sa[45]], [sa[46], sa[47]]], [[sa[48], sa[49]], [sa[50], sa[51]]], [[sa[52], sa[53]], [sa[54], sa[55]]], [[sa[56], sa[57]], [sa[58], sa[59]]], [[sa[60], sa[61]], [sa[62], sa[63]]], [[sa[64], sa[65]], [sa[66], sa[67]]], [[sa[68], sa[69]], [sa[70], sa[71]]]], [[[sa[72], sa[73]], [sa[74], sa[75]]], [[sa[76], sa[77]], [sa[78], sa[79]]], [[sa[80], sa[81]], [sa[82], sa[83]]], [[sa[84], sa[85]], [sa[86], sa[87]]], [[sa[88], sa[89]], [sa[90], sa[91]]], [[sa[92], sa[93]], [sa[94], sa[95]]], [[sa[96], sa[97]], [sa[98], sa[99]]], [[sa[100], sa[101]], [sa[102], sa[103]]], [[sa[104], sa[105]], [sa[106], sa[107]]]], [[[sa[108], sa[109]], [sa[110], sa[111]]], [[sa[112], sa[113]], [sa[114], sa[115]]], [[sa[116], sa[117]], [sa[118], sa[119]]], [[sa[120], sa[121]], [sa[122], sa[123]]], [[sa[124], sa[125]], [sa[126], sa[127]]], [[sa[128], sa[129]], [sa[130], sa[131]]], [[sa[132], sa[133]], [sa[134], sa[135]]], [[sa[136], sa[137]], [sa[138], sa[139]]], [[sa[140], sa[141]], [sa[142], sa[143]]]]])

assert permutedims(po2, (3, 2, 0, 1)) == Array([[[[sa[0], sa[4], sa[8], sa[12], sa[16], sa[20], sa[24], sa[28], sa[32]], [sa[36], sa[40], sa[44], sa[48], sa[52], sa[56], sa[60], sa[64], sa[68]], [sa[72], sa[76], sa[80], sa[84], sa[88], sa[92], sa[96], sa[100], sa[104]], [sa[108], sa[112], sa[116], sa[120], sa[124], sa[128], sa[132], sa[136], sa[140]]], [[sa[2], sa[6], sa[10], sa[14], sa[18], sa[22], sa[26], sa[30], sa[34]], [sa[38], sa[42], sa[46], sa[50], sa[54], sa[58], sa[62], sa[66], sa[70]], [sa[74], sa[78], sa[82], sa[86], sa[90], sa[94], sa[98], sa[102], sa[106]], [sa[110], sa[114], sa[118], sa[122], sa[126], sa[130], sa[134], sa[138], sa[142]]]], [[[sa[1], sa[5], sa[9], sa[13], sa[17], sa[21], sa[25], sa[29], sa[33]], [sa[37], sa[41], sa[45], sa[49], sa[53], sa[57], sa[61], sa[65], sa[69]], [sa[73], sa[77], sa[81], sa[85], sa[89], sa[93], sa[97], sa[101], sa[105]], [sa[109], sa[113], sa[117], sa[121], sa[125], sa[129], sa[133], sa[137], sa[141]]], [[sa[3], sa[7], sa[11], sa[15], sa[19], sa[23], sa[27], sa[31], sa[35]], [sa[39], sa[43], sa[47], sa[51], sa[55], sa[59], sa[63], sa[67], sa[71]], [sa[75], sa[79], sa[83], sa[87], sa[91], sa[95], sa[99], sa[103], sa[107]], [sa[111], sa[115], sa[119], sa[123], sa[127], sa[131], sa[135], sa[139], sa[143]]]]])

# test for large scale sparse array

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 16, 2019

Contributor

also test the existing tests both on sparse and dense arrays.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 1, 2019

Author Contributor

Some tests will fail after including sparse array. The error message is:

AttributeError: 'Tuple' object has no attribute 'as_coeff_Mul'

I will try to solve them.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 2, 2019

Author Contributor

Problem solved! As well as the bug of reshaping sparse array.

kangzhiq added some commits Jul 1, 2019

Updated permutedims
- Simplified codes in permutedims to one line
- Added cases for dict of sparse array in _handle_ndarray_creation_inputs
Added test and fixed bug of reshape sparse array
- Added test for both dense and sparse array in permutedims
- Fixed bug of reshaping a sparse array

@Upabjojr Upabjojr merged commit 21cbaea into sympy:master Jul 3, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.