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

Refactor the sequence interface #12

Merged
merged 1 commit into from
Feb 25, 2014
Merged

Refactor the sequence interface #12

merged 1 commit into from
Feb 25, 2014

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Feb 20, 2014

Refactoring:

Change behavior to be more sane:

  • raise error if step != 1
    • this was silently ignored before

Change behavior to be closer to Python indexing:

  • if start > stop (assuming step==1), an empty array is returned
    • before, the two values were flipped
  • if negative values exceed length, they are set to maximum
    • something strange happened before ...

Refactoring:

* factor-out duplicate code to separate method _get_slice_bounds()
* replace 4 if-clauses with one call to slice.indices()
  * issue #6 is still not solved because __len__() is still broken (#11)

Change behavior to be more sane:

* raise error if step != 1
  * this was silently ignored before

Change behavior to be closer to Python indexing:

* if start > stop (assuming step==1), an empty array is returned
  * before, the two values were flipped
* if negative values exceed length, they are set to maximum
  * something strange happened before ...
@bastibe
Copy link
Owner

bastibe commented Feb 20, 2014

Very nice! Thank you so much for this contribution!
I didn't know about the slice.indices method. That is very useful!

If I understand your commit correctly, you disallow steps that are greater than 1. This seems to mean that I can't SoundFile('file')[100:1000] any more. Is that correct? I would definitely want to allow that.

In fact, I wanted to extend the index operator to channel selections. I would love to be able to SoundFile('file')[0, :44100] to get the first second of the first channel.

@mgeier
Copy link
Contributor Author

mgeier commented Feb 20, 2014

Your example should still work, "step" is the third argument to the slice.

This would not work: SoundFile('file')[100:1000:2]. In the current version, the third argument is just silently ignored, which is not what I would expect.
I simply created an error message for this case.

In the future it would of course be nice to support a step size (for whatever reason this should be needed ...), but this needs a bit more work.
And then, of course, my error message must be removed again.

And I would also like to have the 2-dimensional indexing, this would have been one of my next questions ...
Are you planning to work on it or shall I give it a try?

@bastibe
Copy link
Owner

bastibe commented Feb 25, 2014

Oh, I get it now. Yes, that is perfect!

I am currently pretty short on time, so if you want to give 2-dimensional indexing a shot, I'd be very grateful! I will merge your many contributions tomorrow and recut the binaries. I actually managed to reserve a solid few hours solely for PySoundFile and PySoundCard tomorrow.

bastibe added a commit that referenced this pull request Feb 25, 2014
Refactor the sequence interface
@bastibe bastibe merged commit 015a62e into bastibe:master Feb 25, 2014
@mgeier mgeier deleted the sequence-interface branch February 25, 2014 16:03
@mgeier
Copy link
Contributor Author

mgeier commented Feb 25, 2014

Thanks for merging!

I created an issue for 2D indexing: #15.

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