Closed EdmundButler closed 1 year ago
Thanks for the PR! I haven't checked the changes yet, but a high-level comment: do you want to merge changes directly into master or first go through a developer branch and merge it into master for releases? Either way is fine, the project is small enough that I don't think merging into master directly will cause any headaches tbh - up to you how to manage the branches, I just wanted to check if this is a conscious decision make this PR against master.
Not particularly a conscious decision! But I would prefer to merge directly into master given the small volume of changes.
Mario Sorry about the commented out lines - I had thought I removed them.
And you are absolutely right about consistency. Actually everyone agrees about consistency, but often people do things their own way anyway,. Have you been using a pretty printer/style checker of some sort? That would be the easiest way to remind me to adhere to your style.
Ed
------ Original Message ------ From "Mario Mulansky" @.> To "mariomulansky/PySpike" @.> Cc "Edmund Butler" @.>; "Author" @.> Date 12/14/2022 1:24:24 AM Subject Re: [mariomulansky/PySpike] Support pytest (PR #61)
@mariomulansky commented on this pull request.
very nice, thank you! left some cosmetic comments.
In test/numeric/test_regression_random_spikes.py https://github.com/mariomulansky/PySpike/pull/61#discussion_r1048057816:
@@ -17,10 +18,12 @@
def test_regression_random():
- spike_file = "test/numeric/regression_random_spikes.mat"
- spike_file = os.path.join("test","numeric","regression_random_spikes.mat")
spike_file = os.path.join("numeric","regression_random_spikes.mat")
remove commented code
In test/numeric/test_regression_random_spikes.py https://github.com/mariomulansky/PySpike/pull/61#discussion_r1048060406:
@@ -17,10 +18,12 @@
def test_regression_random():
- spike_file = "test/numeric/regression_random_spikes.mat"
- spike_file = os.path.join("test","numeric","regression_random_spikes.mat") the existing code uses space after comma: ("test", "numeric", "regression_random_spikes.mat") which I believe is the PEP8 recommendation. I'm not going to argue about style guide here, I'm just going to note that consistency helps readability :)
In test/numeric/test_regression_random_spikes.py https://github.com/mariomulansky/PySpike/pull/61#discussion_r1048060486:
spikes_name = "spikes"
result_name = "Distances"
- result_file = "test/numeric/regression_random_results_cSPIKY.mat"
- result_file = os.path.join("test","numeric","regression_random_results_cSPIKY.mat")
result_file = os.path.join("numeric","regression_random_results_cSPIKY.mat")
remove
In test/test_function.py https://github.com/mariomulansky/PySpike/pull/61#discussion_r1048060738:
@@ -147,24 +147,27 @@ def test_pwc_integral(): assert_allclose(f1.integral((2.6,4.0)), (4.0-2.6)0.75) assert_allclose(f1.integral((2.4,4.0)), (2.5-2.4)1.5+(4-2.5)*0.75)
@.(ValueError) @.(ValueError) rm - also below
— Reply to this email directly, view it on GitHub https://github.com/mariomulansky/PySpike/pull/61#pullrequestreview-1216851400, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALRFQNQTN7PR5LZ6TD7AJVTWNFRZRANCNFSM6AAAAAAS5MNQTY. You are receiving this because you authored the thread.Message ID: @.***>
Made suggested style changes
Mario Sorry about the commented out lines - I had thought I removed them. And you are absolutely right about consistency. Actually everyone agrees about consistency, but often people do things their own way anyway,. Have you been using a pretty printer/style checker of some sort? That would be the easiest way to remind me to adhere to your style. Ed …
I see there is a linter check in the CI test script: https://github.com/mariomulansky/PySpike/blob/master/.github/workflows/python-package.yml#L43 But it's commented out - I honestly don't remember why it is commented, or if it was ever active in the first place. Either way, linting the code base and switching this on would be a great help here.
Otherwise this PR is looks good, except some merge conflict?
fixed merge error in test\test_function.py
Stumbling over the GitHub learning curve, I believe I've got it this time.
Mainly changes to avoid deprecation warnings. Also nose.raises replaced with pytest.raises.