florisvb / PyNumDiff

Methods for numerical differentiation of noisy data in python
Other
112 stars 15 forks source link

Issues when running tests #13

Closed pmli closed 2 years ago

pmli commented 2 years ago

I noticed the following issues when running pytest pynumdiff (with Python 3.7.11 and PyNumDiff installed with pip install -e .):

(This is related to my JOSS review: https://github.com/openjournals/joss-reviews/issues/4078)

florisvb commented 2 years ago

I fixed all of these in the most recent push.

Due to a previous issue regarding installing the package I had decided to make the code robust to installing and working as best as possible without installing cvxpy or pychebfun. I think this is the right move, because many of the functions (most, really) work perfectly without these packages, and I don't want to force users to jump through the hoops of installing either of these less common packages.

This does however cause issues when running tests without having these packages installed. My solution, perhaps unorthodox and I am open to suggestions, was to check if cvxpy and pychebfun are installed in the tests that require them, and skip those tests if those packages are not installed. I have logging.info messages for those cases, but pytest does not by default allow those to get printed to the terminal, so they skip silently. I would prefer a warning to show up, I'm open to suggestions there.

All tests pass in python 3.5.2 with cvxpy and pychebfun installed.

In python 3.7.9 without cvxpy or pychebfun installed all tests pass, though the tests that require cvxpy and pychebfun are skipped silently.

florisvb commented 2 years ago

Let me know what you think of my solution for the tests, if its okay I'll close, if not, if you have a suggestion for a better way to handle it that would be very helpful.

pmli commented 2 years ago

Due to a previous issue regarding installing the package I had decided to make the code robust to installing and working as best as possible without installing cvxpy or pychebfun. I think this is the right move, because many of the functions (most, really) work perfectly without these packages, and I don't want to force users to jump through the hoops of installing either of these less common packages.

That sounds good to me.

This does however cause issues when running tests without having these packages installed. My solution, perhaps unorthodox and I am open to suggestions, was to check if cvxpy and pychebfun are installed in the tests that require them, and skip those tests if those packages are not installed. I have logging.info messages for those cases, but pytest does not by default allow those to get printed to the terminal, so they skip silently. I would prefer a warning to show up, I'm open to suggestions there.

I would suggest using pytest.skip instead of logging. That way, pytest will report how many tests were skipped.

All tests pass in python 3.5.2 with cvxpy and pychebfun installed.

For me, with Python 3.7.11, one test fails:

    def test_iterative_velocity(self):
        params_1, val_1 = iterative_velocity(x, dt, params=None, tvgamma=tvgamma, dxdt_truth=dxdt_truth)
        params_2, val_2 = iterative_velocity(x, dt, params=None, tvgamma=0, dxdt_truth=None)
>       self.assertListEqual(params_1, [2, 0.0001])
E       AssertionError: Lists differ: [1, 0.0001] != [2, 0.0001]
E       
E       First differing element 0:
E       1
E       2
E       
E       - [1, 0.0001]
E       ?  ^
E       
E       + [2, 0.0001]
E       ?  ^

pynumdiff/tests/test_optimize.py:100: AssertionError
florisvb commented 2 years ago

I replaced the logging calls with pytest.skip, and I tried to make the test that failed for you more robust. Hard for me to fully debug though, since I get a different value (i.e. 2 instead of 1). What version of scipy and numpy do you have?

pmli commented 2 years ago

I replaced the logging calls with pytest.skip,

Great, it looks good.

and I tried to make the test that failed for you more robust. Hard for me to fully debug though, since I get a different value (i.e. 2 instead of 1). What version of scipy and numpy do you have?

I used the versions from requirements.txt.

florisvb commented 2 years ago

That is odd, I'm using the same versions of scipy and numpy, on python 3.7.

In any case, could you rerun the tests with the latest code to see if it passes now?

pmli commented 2 years ago

Yes, the relaxed test passes.

florisvb commented 2 years ago

Excellent, thanks.

I believe everything from this issue is now addressed, so I'm going to close the issue. If I missed something let me know.