Several changes to the read() and write() methods (alternative) by bastibe · Pull Request #37 · bastibe/python-soundfile

bastibe · 2014-05-09T14:14:39Z

This implements several changes to the read() and write() methods.

Add always_2d argument to read()
Add out argument to read()
Add fill_value argument to read()
Add read() and write() functions

This is the combination of a few commits from #18. See also #14.

Replace ffi.new() with np.empty() and np.ascontiguousarray() Remove dicts for readers/writers read(): * reserve only as much memory as needed (if 'frames' is too large) * check return value of sf_readf_*() write(): * avoid copy if data has already the correct memory layout * check return value of sf_writef_*() * don't return the number of written frames! This is for symmetry with read() and it's redundant information anyway As the written frames are now checked in the write() method, the same check was removed from the write() *function*.

See also #16

If less frames are left in the file than would fit into out and fill_value=None, a smaller view into out is returned that contains only the valid frames.

- pysoundfile is not imported into the global namespace any longer, since that would overwrite `open`. - Split the test class into subclasses

- removed `channels_first`. - split read into `read` and `readinto`. - removed many helper functions that were not needed any longer. - added tests for `fill_value` and EOF behavior.

readinto can thus be turned into a private method

bastibe · 2014-05-09T14:19:07Z

you call self.seek(0, SEEK_CUR, 'r') three times (reducing readability/maintainability, the performance hit is of course negligible)

in some cases too much memory is allocated

I don't like how you create out and then conditionally change its shape. I prefer to get the correct shape in the first place and then create an array with it. Also self.channels == 1 is more readable than out.shape[1] == 1.

fill_value is assigned even if it's None and the view to assign to is actually empty. This doesn't (yet) work on PyPy.

These issues should be addressed now.

This is a fork of #34, which does not need the _check_frames_and_channels helper method and is more compact in general.

Also, this fork is mostly tested.

mgeier · 2014-05-11T17:27:10Z

This is starting to get confusing ...

This is a fork of #34

#34 is quite outdated, because I created #36 (following your suggestion, @bastibe).
I thought we wanted to merge the changes to the methods first, and then tackle the functions separately.

Currently it's hard to see the differences regarding the functions.
Do you want me to re-combine #34 and #36 or do you want to remove the functions from #37?

mgeier · 2014-05-11T18:05:43Z

pysoundfile.py

This is the third time that self.frames - current_frame is calculated (the others being in line 622 and 624). I guess this is a code smell.

You said the same somewhere else. I'll amend it.

bastibe · 2014-05-11T18:30:03Z

I fixed most of your comments. Thank you!

BTW, that test suite is invaluable!

bastibe · 2014-05-11T18:52:03Z

Sorry about the confusion with #34, #36, and #37. This pull request was supposed to be for discussion purposes mostly. I think we did settle on an API for SoundFile.read and SoundFile.write. Now we have to come up with an implementation. Please ignore the read and write functions for the time being if you want.

mgeier · 2014-05-12T07:57:38Z

OK, I can ignore the functions.

I found this quite interesting:

git difftool origin/open-read-write-alternative  origin/read-write pysoundfile.py

Also, according tests

bastibe · 2014-05-12T09:44:58Z

Very interesting indeed. I missed the fact that there are many similarities between the error checking in SoundFile.write and SoundFile.read. Thus, I kind of re-implemented your _check_frames_and_channels, though this time only for error checking.

Actually, this made SoundFile.read quite a bit shorter. What do you think about this approach?

mgeier · 2014-05-17T13:33:36Z

I like the idea of re-factoring out common functionality of read() and write() to make them both shorter.

However, I found a different (but overlapping) set of functionality to factor out.
See d73ef60 in #36.

I guess the name could be improved, though (_read_or_write()) ...

bastibe · 2014-05-27T11:13:19Z

This has been superceded by #36

@bastibe

See also #16 If less frames are left in the file than would fit into out and fill_value=None, a smaller view into out is returned that contains only the valid frames. This includes several suggestions by @bastibe from #37.

mgeier and others added 18 commits April 21, 2014 23:17

Add open(), read() and write() functions

1f22833

This is the combination of a few commits from #18. See also #14.

Add arguments channels_first, always_2d, out and fill_value

202fe2b

See also #16

Change handling of out argument

5d7236c

If less frames are left in the file than would fit into out and fill_value=None, a smaller view into out is returned that contains only the valid frames.

Add helper function _check_frames_and_channels()

f64da11

refactored tests for improved clarity

ace9936

- pysoundfile is not imported into the global namespace any longer, since that would overwrite `open`. - Split the test class into subclasses

Simplified read and write

49a16c8

- removed `channels_first`. - split read into `read` and `readinto`. - removed many helper functions that were not needed any longer. - added tests for `fill_value` and EOF behavior.

refactored read to accept out

db2cb0b

readinto can thus be turned into a private method

re-integrated read and readinto

a700fcf

Fixed documentation of test case

161f97e

Added tests for out argument of read

94c2a00

removed duplicated self.seek(0, SEEK_CUR, 'r')

564e9bc

no more reshaping out after creation

1a154da

no more allocating of unused memory

4d28a10

no more returning views of all the data

28527a7

removed unnecessary nesting

5c68a04

fixed write function

877fc3a

fixed write function for real and test it

b56606e

bastibe mentioned this pull request May 9, 2014

Add open(), read() and write() functions #34

Merged

mgeier reviewed May 11, 2014
View reviewed changes

bastibe added 2 commits May 11, 2014 20:27

some refactorings to improve clarity

1d3a217

fixed broken docstring

6f43d1c

added test for read with empty out

d331c89

bastibe added 3 commits May 12, 2014 10:53

renamed read_frames to frames_to_read

06599da

Added checks for out dimensionality

7471637

Also, according tests

refactored error checking into its own method

042d425

mgeier mentioned this pull request May 17, 2014

Several changes to the read() and write() methods #36

Merged

This was referenced May 20, 2014

Some improvements to the virtual IO code #38

Merged

Move module constants into cdef #39

Merged

bastibe closed this May 27, 2014

bastibe added a commit that referenced this pull request May 27, 2014

Updated tests to the latest version from #37

7e01361

bastibe mentioned this pull request May 27, 2014

Updated tests #40

Merged

mgeier mentioned this pull request Jun 17, 2014

read(): don't ignore frames if out is given? #46

Merged

bastibe added a commit that referenced this pull request Dec 17, 2014

Updated tests to the latest version from #37

64ab6bb

bastibe deleted the open-read-write-alternative branch April 25, 2016 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several changes to the read() and write() methods (alternative)#37

Several changes to the read() and write() methods (alternative)#37
bastibe wants to merge 24 commits intomasterfrom
open-read-write-alternative

bastibe commented May 9, 2014

Uh oh!

bastibe commented May 9, 2014

Uh oh!

mgeier commented May 11, 2014

Uh oh!

mgeier May 11, 2014

Uh oh!

bastibe May 11, 2014

Uh oh!

bastibe commented May 11, 2014

Uh oh!

bastibe commented May 11, 2014

Uh oh!

mgeier commented May 12, 2014

Uh oh!

bastibe commented May 12, 2014

Uh oh!

mgeier commented May 17, 2014

Uh oh!

bastibe commented May 27, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bastibe commented May 9, 2014

Uh oh!

bastibe commented May 9, 2014

Uh oh!

mgeier commented May 11, 2014

Uh oh!

mgeier May 11, 2014

Choose a reason for hiding this comment

Uh oh!

bastibe May 11, 2014

Choose a reason for hiding this comment

Uh oh!

bastibe commented May 11, 2014

Uh oh!

bastibe commented May 11, 2014

Uh oh!

mgeier commented May 12, 2014

Uh oh!

bastibe commented May 12, 2014

Uh oh!

mgeier commented May 17, 2014

Uh oh!

bastibe commented May 27, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants