Minor updates #1

Merged
merged 7 commits into from Jun 19, 2013

Projects

None yet

2 participants

@jpf
Contributor
jpf commented Jun 17, 2013

I added a .gitignore, because it's useful.

I use virtualenv, so I added a requirements.txt file so that I can do a pip install -r requirements.txt to get PIL

I also also changed the comments in sstv.py to reflect the explanations that you put in README.md - they are much more clear and very useful to see in the code.

@dnet dnet and 1 other commented on an outdated diff Jun 18, 2013
@@ -0,0 +1,2 @@
+*.py[cod]
@jpf
jpf Jun 18, 2013 Contributor

I added these because it seems to be convention for the projects that I've seen. That said, I'll take your advice to set up a global gitignore and remove the .gitignore from my pull request.

@dnet dnet commented on an outdated diff Jun 18, 2013
@@ -0,0 +1,2 @@
+*.py[cod]
+venv
@dnet
dnet Jun 18, 2013 Owner

this should go to your local gitignore in .git/info/exclude since the code doesn't refer to venv anywhere, so I assume you used it to store a virtual environment or something like that

@dnet dnet and 1 other commented on an outdated diff Jun 18, 2013
tests/test_sstv.py
+ (1300, 30),
+ (1200, 30)]
+ actual = list(islice(gen_freq_bits, 0, 1000))
+ self.assertEqual(expected, actual)
+
+ # FIXME: Instead of using a test fixture, 'expected' should be synthesized?
+ def test_gen_values(self):
+ gen_values = self.s.gen_values()
+ expected = pickle.load(open("tests/assets/SSTV_gen_values.p"))
+ actual = list(islice(gen_values, 0, 1000))
+ self.assertEqual(expected, actual)
+
+ def test_gen_samples(self):
+ gen_values = self.s.gen_samples()
+ # I expected to need this, but I don't? Not in this instance?
+ # sstv.random = Mock(return_value=0.4) # xkcd:221
@dnet
dnet Jun 18, 2013 Owner

It requires random because I avoided quantization noise using additive noise, so there's always a chance of running the code two consecutive times on the same machine and having different results, but only by ±1. If you'd like to do tests like this, it'd be better to initialize random (within the SSTV class) in a way that the output is predictable.

@jpf
jpf Jun 18, 2013 Contributor

Ah, very cool. Thanks for letting me know. This project is my first exposure to digital signal processing, so I'm learning as I go along.

@dnet dnet commented on an outdated diff Jun 18, 2013
tests/test_sstv.py
+ # sstv.random = Mock(return_value=0.4) # xkcd:221
+ expected = pickle.load(open("tests/assets/SSTV_gen_samples.p"))
+ actual = list(islice(gen_values, 0, 1000))
+ self.assertEqual(expected, actual)
+
+ def test_write_wav(self):
+ self.maxDiff = None
+ sio = StringIO()
+ sio.close = MagicMock() # ignore close()
+ mock_open = MagicMock(return_value=sio)
+ with mock.patch('__builtin__.open', mock_open):
+ self.s.write_wav('unittest.wav')
+ expected = 'bf61c82e96aed1370d5c1753d87729db'
+ data = sio.getvalue()
+ hash = hashlib.md5()
+ hash.update(data)
@dnet
dnet Jun 18, 2013 Owner

hashlib.md5(data) would've achieved the same

@dnet
Owner
dnet commented Jun 18, 2013

Thanks for the pull request, I agree with most of it. I put some comments in the code, if you could deal with those, I'll merge your branch.

@jpf
Contributor
jpf commented Jun 18, 2013

Cool! I'll make the changes you've suggested and send you a pull request when I do.

@jpf
Contributor
jpf commented Jun 18, 2013

Alright. Updates made. Let me know if I missed anything.

@dnet dnet merged commit f14d0a8 into dnet:master Jun 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment