Testing #61

trojanjay · 2018-06-21T18:10:06Z

What does this PR do?

Tests cart2sph & sph2cart functions

mgeier · 2018-06-22T07:44:55Z

Thanks, the parametrization is a very nice improvement compared to #60!

Why did you change the tolerance of assert_allclose()?
I'm not saying it's wrong, I'm just wondering ...

There are some minor coding style issues, could you please have a look at the output of this?

python3 -m pycodestyle tests/test_util.py

mgeier · 2018-06-22T07:49:11Z

tests/test_util.py

+    ((1,1,-1), (np.pi / 4, np.arccos(-1/np.sqrt(3)), np.sqrt(3))),
+    ((-1,1,-1), (np.arctan2(1,-1), np.arccos(-1/np.sqrt(3)), np.sqrt(3))),
+    ((1,-1,-1), (-np.pi / 4, np.arccos(-1/np.sqrt(3)), np.sqrt(3))),
+    ((-1,-1,-1), (np.arctan2(-1,-1), np.arccos(-1/np.sqrt(3)), np.sqrt(3)))


When having a list with one entry per line, it's considered good style to add a final comma to the last line, even though this is not strictly necessary. This makes it easier to add new lines later. It also makes it easier to re-arrange the lines if that's desired at some point.

Thank you for the feedback. I will explore pycodestyle to correct the coding style.

about assert_allclose(), I had a precision higher than the default so I decided to exploit it.

spors · 2018-06-22T08:40:29Z

tests/test_util.py

+import pytest
+import sfs
+
+test_data = [


Since we might add more tests for ùtil` in the future the name of the array should be choosen more specific.

Noted! I will change it to reflect the test it is referenced in.

mgeier · 2018-06-23T18:45:41Z

@trojanjay

about assert_allclose(), I had a precision higher than the default so I decided to exploit it.

I see, but in fact you were reducing the precision, because the default is atol=0, see https://docs.scipy.org/doc/numpy/reference/generated/numpy.testing.assert_allclose.html.
Did you perhaps mean numpy.isclose() (https://docs.scipy.org/doc/numpy/reference/generated/numpy.isclose.html) which has atol=1e-08 as default?

I was quite confused when I discovered the different default values some time ago ...
I think that atol=0 is a bit restrictive, but probably that has some good reason?

trojanjay · 2018-06-24T14:58:23Z

@mgeier Thank you very much for this observation.

The error I made was not setting rtol =0. The function works with both rtol (tolerance per unit value) and atol (overall tolerance). In my case atol is more important to me not rtol.

I am still struggling to understand pycodestyle. I do not know how to interpret the output, can you give more insight please.

z.B. tests/test_util.py:12:32: E231 missing whitespace after ',' - how do I know on which line of code to correct?

mgeier · 2018-06-25T20:44:31Z

In my case atol is more important to me not rtol.

Are you sure?
The error will depend on the magnitude of the values you are using in the test, right?
What if you try coordinates with a few million meters?

I thought that atol is only needed when the result is supposed to be zero, in which case the rtol would become useless.

Anyway, I would only use a non-default value if there is a good reason.
And if there is, it would probably make sense to add a comment explaining this reason?

I am still struggling to understand pycodestyle

Well first of all you should read PEP8: https://www.python.org/dev/peps/pep-0008/
This should explain most of the possible messages.

pycodestyle is just an attempt to automatically check for those (sometimes intentionally fuzzy) guidelines. You should know that you don't have to follow those 100% of the time, but in most cases those are good suggestions.

how do I know on which line of code to correct?

The two numbers after the file name are the line and column numbers, respectively.

But you can just look at all commas and check if there is proper whitespace after them.
But again, you should read PEP8 first ...

Some editors also have the ability to check this in the background and show the warnings similar to a spell checker. Probably there is a plugin available for your editor?
BTW, you could also check if your editor has a plugin for the .editorconfig file: https://editorconfig.org/#download

trojanjay · 2018-10-08T14:11:31Z

I didn't properly understand the reason why the coordinates of the image sources are ordered the way they are ordered. The actual values are correct, I just don't why it is arranged the way it is. I think if I properly understands this, it will help in extending to more than the first image sources.

Also, I did not understand why the format for the wall count is the way it is.

Can anyone give me a little insight so I do not run into too much problems in the future?

mgeier · 2018-10-09T11:13:09Z

Thanks for the new commits!

I didn't see your older commits f1cbe7e and a0637ef.

In the future, please write a comment when you have done some suggested changes, to make sure that everyone notices them.
And if there is no answer for, let's say, about a week, please write a reminder comment until somebody answers.

I've squashed and merged your older commits and made an additional commit changing the test function names from test* to test_*.

Can you please rebase your new commit on the new master branch?
If you don't know how to do that, you can just simply create a new branch from master and copy your changes there and make a new PR.

I didn't look into image_sources_for_box() yet, so I cannot help for the moment, but probably somebody else can?

Writing the tests already paid off, because you found a bug in the db() function!
If you know how to do this, you can fix the error. If not, please let me know.

mgeier

I've added a few comments.

mgeier · 2018-10-09T11:13:53Z

tests/test_util.py

+
+
+@pytest.mark.parametrize('linear, decibel', db_data)
+def testdb(linear, decibel):


Please start test function names with test_.

mgeier · 2018-10-09T11:15:49Z

tests/test_util.py

+
+
+db_data = [
+    ((0), -math.inf),


The parentheses in (0) have no effect. They are especially not creating a tuple, which it looks like you wanted to do?

See https://nbviewer.jupyter.org/github/mgeier/python-audio/blob/master/intro-python.ipynb#Tuples

mgeier · 2018-10-09T11:17:17Z

tests/test_util.py

+
+
+db_data = [
+    ((0), -math.inf),


You could use -np.inf, then you don't need the separate math import.

mgeier · 2018-10-09T11:20:10Z

tests/test_util.py

+def testdb(linear, decibel):
+    try:
+        x, is_power = linear
+    except:


I don't really like mixing tuples and numbers in the parameters.

Why don't you split this into two test functions, one for power=False and the other for power=True?

You could even re-use the test data in this case and just divide the expected result by 2 for power=True.

mgeier · 2018-10-09T11:31:25Z

tests/test_util.py

+        is_power = False
+
+    d = sfs.util.db(x, is_power)
+    assert_allclose(d, decibel, rtol=1e-2)


I'm not sure if it's a good idea to use such a big tolerance.

I think it would be better if you just calculate some values with the required accuracy and then use the default tolerances to test it.

Or is there a reason not to do that?

…ctor rev1

trojanjay · 2018-10-11T10:20:31Z

@mgeier Thank you for the feedback. I have implemented your suggestions.

mgeier · 2018-10-11T13:21:10Z

Thanks for the update! And thanks for fixing the db() function!

Now there are some pycodestyle issues again:

$ python3 -m pycodestyle tests/test_util.py
tests/test_util.py:87:8: E128 continuation line under-indented for visual indent
tests/test_util.py:96:80: E501 line too long (89 > 79 characters)

mgeier · 2018-10-11T13:21:26Z

.gitignore

 .eggs/
 sfs.egg-info/
+.cache
+.vscode


What are these for?

Don't know about .cache but .vscode is the project folder of visual studio code. I personally also use it but I don't know what the policy on adding stuff like this to the .gitignore

I guess different people use different policies.
I like to keep the file short and clean. Others like horribly complicated ones like https://github.com/github/gitignore/blob/master/Python.gitignore.
I think it should only contain things that are relevant to the current project and programming language(s).

User-dependent things like editor settings and stuff like this should IMHO go into your own global ignore file, see https://help.github.com/articles/ignoring-files/#create-a-global-gitignore and https://stackoverflow.com/a/7335487/.

For example, I have those in my global ignore list:

tags core octave-core .*.swp *.un~ .ipynb_checkpoints/ .ycm_extra_conf.pyc __pycache__/

I didn't intend to put it as part of a pull request. Sorry!

mgeier · 2018-10-11T13:23:44Z

tests/test_util.py

+    (10*4, (10+6.02059991327962)),
+    (10*0.5, (10-3.01029995663981198)),
+    (10*0.1, (10-10)),
+    (10*0.25, (10-6.02059991327962396))


This looks much better now.

But I don't really understand why you are using 10*... and 10+.... Can you please explain?

This are kind of known identities when converting from linear scale to dB.

Such that when you double in linear scale = + 3(approx.) in dB etc.

I put in the code so that it is obvious to anyone.

OK, I see.

So why don't you drop the 10?

You could just use

... (0.5, -3.01029995663981198), ... (2, 3.010299956639813), ...

IMHO it would also be easier to read if you sort the lines.

Also, you really don't need that many test cases, I think about half of them would be enough.

But you could make a separate test function to test one case where the function raises an error, namely using a negative input.

mgeier · 2018-10-11T13:26:44Z

tests/test_util.py

+@pytest.mark.parametrize('linear, decibel', db_data)
+def test_db_power(linear, decibel):
+    d = sfs.util.db(linear)
+    assert_allclose(d, 2*decibel)


This is not required by PEP8, but I personally would prefer spaces around the * here.

mgeier · 2018-10-15T18:01:40Z

tests/test_util.py

+
+@pytest.mark.parametrize('linear, decibel', db_data)
+def test_db_amplitude(linear, decibel):
+    d = sfs.util.db(linear, True)


This doesn't match the function name.

Also, you should not use a bare True or False as a function argument (unless probably if it's the only argument).
You should use it as a keyword argument, i.e. you should use power=True (except you should actually use that in test_db_power()).

mgeier · 2018-10-15T18:12:06Z

tests/test_util.py

+    (10*4, (10+6.02059991327962)),
+    (10*0.5, (10-3.01029995663981198)),
+    (10*0.1, (10-10)),
+    (10*0.25, (10-6.02059991327962396))


OK, I see.

So why don't you drop the 10?

You could just use

... (0.5, -3.01029995663981198), ... (2, 3.010299956639813), ...

IMHO it would also be easier to read if you sort the lines.

Also, you really don't need that many test cases, I think about half of them would be enough.

But you could make a separate test function to test one case where the function raises an error, namely using a negative input.

This is a part of #61

mgeier · 2019-02-05T19:36:37Z

I've taken most of this and used it in #78.

This is a part of #61

trojanjay added 2 commits June 18, 2018 23:19

pytest for cart2sph & sph2cart

7222a17

Tests for sph2cart & cart2sph

89dabda

mgeier reviewed Jun 22, 2018

View reviewed changes

mgeier mentioned this pull request Jun 22, 2018

pytest for cart2sph & sph2cart #60

Closed

spors reviewed Jun 22, 2018

View reviewed changes

trojanjay added 3 commits July 1, 2018 17:01

pytest for Sph2Cart & Cart2Sph

f1cbe7e

pytest for Sph2Cart & Cart2Sph

a0637ef

Test for util.db, util.direction_vector & util.image_source_for_box

83e85d9

mgeier reviewed Oct 9, 2018

View reviewed changes

trojanjay added 2 commits October 11, 2018 12:07

Test for util_image_sources_for_box_data, util_db & util_direction_ve…

5db42bc

…ctor rev1

Merge branch 'master' into testing

61e0e07

mgeier reviewed Oct 11, 2018

View reviewed changes

trojanjay added 2 commits October 15, 2018 20:06

Comment on pycodestyle incorporated

0c10724

Cleaning .gitignore

5667262

mgeier reviewed Oct 15, 2018

View reviewed changes

mgeier pushed a commit that referenced this pull request Feb 5, 2019

Test for util.db, util.direction_vector

08bd428

This is a part of #61

mgeier mentioned this pull request Feb 5, 2019

Test updates #78

Merged

mgeier closed this Feb 5, 2019

mgeier pushed a commit that referenced this pull request Feb 10, 2019

Test for util.db, util.direction_vector

b676d2f

This is a part of #61



		@pytest.mark.parametrize('linear, decibel', db_data)
		def testdb(linear, decibel):

Testing #61

Testing #61

Uh oh!

Conversation

trojanjay commented Jun 21, 2018

What does this PR do?

Uh oh!

mgeier commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spors Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeier commented Jun 23, 2018

Uh oh!

trojanjay commented Jun 24, 2018

Uh oh!

mgeier commented Jun 25, 2018

Uh oh!

trojanjay commented Oct 8, 2018

Uh oh!

mgeier commented Oct 9, 2018

Uh oh!

mgeier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trojanjay commented Oct 11, 2018

Uh oh!

mgeier commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeier commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spors Jun 22, 2018 •

edited

Loading