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

[WIP]Implemented LIL format for sparse array #17160

Open
wants to merge 7 commits into
base: master
from

Conversation

@kangzhiq
Copy link
Contributor

kangzhiq commented Jul 7, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

The idea comes from the standard format of sparse matrix. Would it be interesting as well to implement such data structure in sparse array so that some operations can be more efficient? The current format that Sparse Array module uses is DOK.

The format LIL is efficient in incremental construction.
This experimental implementaiton of LIL algorithm will help verify the possibility and advantage of implementing different format to sparse array.

Other comments

Release Notes

  • tensor
    • Added algorithm for data strucutre of sparse array
- Added new class lil, initial version
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 7, 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

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. -->


#### Brief description of what is fixed or changed
The idea comes from the standard format of sparse matrix. Would it be interesting as well to implement such data structure in sparse array so that some operations can be more efficient? The current format that Sparse Array module uses is DOK.

The format LIL is efficient in incremental construction. 
This experimental implementaiton of LIL algorithm will help verify the possibility and advantage of implementing different format to sparse array.  

#### Other comments


#### 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 algorithm for data strucutre of sparse array 
<!-- END RELEASE NOTES -->


if isinstance(iterable, SparseNDimArray):
new_dict = iterable._sparse_array
# if it is a dense array, it would be cast to a default sparse array

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 7, 2019

Author Contributor

The idea comes from SciPy where all sparse array are cast to a CSR format before converting to other format.

raise NotImplementedError("Data type not yet supported")

# Add non zero value to internal list.
# This operation can be simplified once Array module supports tuple index

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 7, 2019

Author Contributor

This is related to the problem of NumPy-like behavior in Array module. What I was trying to do is:

>>> A = Array(iterable, (3, 4, 3))
>>> index = (1, 2, 0)
>>> A[index[:-1]]

which is suppposed to return A[1, 2, :].
But currently, Array module in SymPy doesn't handle the tuple as I wished.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

It should be extended.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 25, 2019

Contributor

I mean... if index[:-1] is not supported... maybe support for it should be added.

1
>>> a[0, 1] = 3
>>> a.tolist()
[[0, 3, 0], [2, 0, 0]]

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 7, 2019

Author Contributor

Only some basic operations are tested. I have briefly tested addition and multiplication, it didn't work but there was not an error either. I will check it later. :-)

"""
def __new__(cls, iterable=None, shape=None, **kwargs):
shape, flat_list = cls._handle_ndarray_creation_inputs(iterable, shape, **kwargs)
shape = Tuple(*map(_sympify, shape))

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 7, 2019

Member

Tuple.fromiter possible?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 12, 2019

Author Contributor

Yes, thank you for this advice!

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 8, 2019

I see you have implemented the whole class. I was expecting just the algorithm.

kangzhiq added 2 commits Jul 14, 2019
- Ameliorated implementation of lil
- Added implementation of coo after testing
- Added basic implementation of csr
- Implemented basic structure of csr format
- Added some illustration for each format
return all([shape[i] > index[i] for i in range(len(shape))])

# Row-based linked list sparse matrix(LIL)
class lil(Basic, SparseNDimArray):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 16, 2019

Contributor

We need a better name.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 17, 2019

Author Contributor

What about LilArray? Or maybe LilSparseArray

return 0

# For mutable arrays
def __setitem__(self, index, value):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 16, 2019

Contributor

This should raise an exception if the class inherits Basic

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 17, 2019

Author Contributor

Yes, it is a little bit contradictory to handle both immutable and mutable property in one class. I was thinking that we can handle this property by adding a flag: isMutable, but it turns out that it is not very pratical.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 17, 2019

Contributor

No, it cannot be done. The reason is complicated but SymPy needs all of its objects to be immutable. If you want a mutable object, you need to create a separate class.

Have a look at the existing class structures.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 16, 2019

Where are the tests?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Jul 16, 2019

If the class is mutable, it should not be a SymPy object.

I see a lot of code that could be better reorganized.

@kangzhiq

This comment has been minimized.

Copy link
Contributor Author

kangzhiq commented Jul 17, 2019

I am working on the amelioration of code and adding the tests

@kangzhiq

This comment has been minimized.

Copy link
Contributor Author

kangzhiq commented Jul 17, 2019

Where are the tests?
I see a lot of code that could be better reorganized.

I am working on the amelioration of code and adding the tests

kangzhiq added 2 commits Jul 19, 2019
- Added tests for new class
- Ameliorate code for new class
- Added new way to initialize a new format, which can facilitate the
convertion of different format
- Added an example of converint COO to CSR
- Added tests accordingly
return self.toscr()._mul(other)

def __setitem__(self, index, value):
raise TypeError('Immutable N-dim array')

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

what about moving this to the Immutable... superclass?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 24, 2019

Author Contributor

Yes, this line of code is also used in ImmutableDenseNDimArray, ImmutableSparseNDimArray , removing it to ImmutableNDimArray will avoid repetitive code.

raise NotImplementedError("Data type not yet supported")

# Add non zero value to internal list.
# This operation can be simplified once Array module supports tuple index

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

It should be extended.

kangzhiq added 2 commits Aug 4, 2019
- Removed inheritance of Basic
- Added conversion from LIL to CSR format
- Added tests accordingly
@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented Aug 23, 2019

There is a conflict with the current master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.