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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request : slicing with lists #2

Closed
wants to merge 1 commit into from

Conversation

Goutte
Copy link

@Goutte Goutte commented May 12, 2016

Great lib ! I really like the elegance of it. That's good cheese. 馃嵎

I hacked support for the following tests :

    def test_slice_with_list_read(self):
        sl = sparse_list.SparseList([1, 2, 3, 4, 5])
        ip = [0, 2, 4]
        self.assertEquals([1, 3, 5], sl[ip])

    def test_slice_with_list_write(self):
        sl = sparse_list.SparseList([1, 2, 3, 4, 5])
        ip = [0, 2, 4]
        sl[ip] = [6, 7, 8]
        self.assertEquals([6, 2, 7, 4, 8], list(sl))

I wrote tests for numpy too (just to be sure) but decided against including them in a PR, unless you want them :

    def test_slice_with_numpy_array_read(self):
        import numpy
        sl = sparse_list.SparseList([1, 2, 3, 4, 5])
        ip = numpy.array([0, 2, 4])
        self.assertEquals([1, 3, 5], sl[ip])

    def test_slice_with_numpy_array_write(self):
        import numpy
        sl = sparse_list.SparseList([1, 2, 3, 4, 5])
        ip = numpy.array([0, 2, 4])
        sl[ip] = [6, 7, 8]
        self.assertEquals([6, 2, 7, 4, 8], list(sl))

I have a bunch of questions and remarks, while I'm at it (you don't have to answer, but given your SO score I guess explaining does not bother you too much :3) :

  1. Why triple single quotes ''' and not double quotes """ for doctrings ?
  2. There is a SyntaxError in setup.py with python 2.7, because of the positional argument install_requires('future', 'six'),
  3. There's a fromage called benchmark whose simplicity and absence of code in multiline strings you might enjoy, if you don't already know about it.
  4. I rather enjoyed your sense of humour, thank you for that too !
  5. I know that comments are a failure but I took the liberty of adding two of them, you'll see. (and possibly remove them, I won't mind)

(works with numpy ndarrays too)
@Goutte Goutte mentioned this pull request May 13, 2016
@@ -125,22 +135,22 @@ def __mul__(self, multiplier):

def count(self, value):
'''
return number of occurrences of value
Return the number of occurrences of value.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't recall now, but I think I copied the language from the official documentation for list...

> pydoc list | fgrep -A1 count
 |  count(...)
 |      L.count(value) -> integer -- return number of occurrences of value
 |  

> pydoc sparse_list | fgrep -A1 count
     |  count(self, value)
     |      return number of occurrences of value

Copy link
Author

Choose a reason for hiding this comment

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

Urgh. I hesitated for a while before changing the text, but then my inner grammarian assumed control, i guess. I can revert it back, I don't know how to choose between consistency with the existing doc and literacy. You choose. :)

@johnsyweb
Copy link
Owner

Hi @Goutte!

Thank you very much for this PR! It has been nice to revisit this project after having neglected it for so long. I don't think I've written any Python code since I wrote this module about three years ago, so my memory around this is a little faded. Apologies for my vagueness.

To address your questions and remarks:

Why triple single quotes ''' and not double quotes """ for doctrings ?

I think the convention at my previous employer was to use single quotes for strings wherever possible. I've been doing Ruby for so long now, I can't recall whether there was any difference in Python (like there is in Ruby). Were I to start again, I would probably adhere to http://legacy.python.org/dev/peps/pep-0257/ 馃槃

There is a SyntaxError in setup.py with python 2.7, because of the positional argument install_requires('future', 'six'),

Interesting. I wonder how I didn't pick that up. Perhaps I was using an older version of Python 2.7 than we have now.

There's a fromage called benchmark whose simplicity and absence of code in multiline strings you might enjoy, if you don't already know about it.

Thanks for this. I would certainly use benchmark instead of my dodgy time_sparse_list script.

I rather enjoyed your sense of humour, thank you for that too !

馃槉

I know that comments are a failure but I took the liberty of adding two of them, you'll see. (and possibly remove them, I won't mind)

He he. Thanks for reading that article!

johnsyweb added a commit that referenced this pull request Nov 11, 2016
Addresses #2
@johnsyweb
Copy link
Owner

Looks like https://github.com/Goutte/python_sparse_vector is the bomb when it comes to numpy! 馃挜

@johnsyweb johnsyweb closed this Nov 11, 2016
johnsyweb pushed a commit that referenced this pull request Jun 14, 2018
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.

None yet

2 participants