fix 'null' in lower case as a missing value in the new Yahoo API, make compatible with the older pandas versions #357

OlegShteynbuk · 2017-07-03T03:19:16Z

closes #342

add 'null' in lower case to na_values argument to read_csv

jreback · 2017-07-03T03:24:20Z

can u add the test from the issue so that we can see that this works

OlegShteynbuk · 2017-07-03T03:26:18Z

add 'null' in lower case to na_values arguments

OlegShteynbuk · 2017-07-03T04:48:57Z

the test added

jreback

pls add a note in the whatsnew for 0.5

jreback · 2017-07-03T07:16:40Z

pandas_datareader/tests/yahoo/test_yahoo.py

@@ -132,6 +132,11 @@ def test_get_data_multiple_symbols(self):
        web.get_data_yahoo(sl, '2012')

    @skip_on_exception(RemoteDataError)
+    def test_get_data_null_as_missing_data(self):
+        # just test that we succeed


add both cases and assert the dtypes of the result

OlegShteynbuk · 2017-07-03T15:07:02Z

a note in the whatsnew for 0.5 added

jreback

small test change, otherwise lgtm.

jreback · 2017-07-04T00:21:21Z

pandas_datareader/tests/yahoo/test_yahoo.py

@@ -132,6 +132,18 @@ def test_get_data_multiple_symbols(self):
        web.get_data_yahoo(sl, '2012')

    @skip_on_exception(RemoteDataError)
+    def test_get_data_null_as_missing_data_no_adjust(self):


parametrize this with adjust_prices=True & False (and remove the test below)

jreback · 2017-07-04T00:22:41Z

FYI we are testing on older pandas versions and such so no special handling, everything should work. null should be fine in newer (as its default) and older (as you added it).

OlegShteynbuk · 2017-07-05T13:22:09Z

Added parametrize. In order to use parametrize created an inner function(added a level of indirection) because skip_on_exception decorator changes the signature. The other solution would be to make skip_on_exception decorator a signature-preserving decorator. But this is outside of the scope of this PR as it will affect other tests; my first impression is that it will affect other tests for the better as will allow using skip_on_exception decorator with parametrize, it could be a little bit slower but the difference is rather small and for testing it should not be an issue. I can go ahead and create a new issue.

bashtage · 2017-07-05T13:32:44Z

pandas_datareader/tests/yahoo/test_yahoo.py

@@ -133,7 +133,7 @@ def test_get_data_multiple_symbols(self):

    @pytest.mark.parametrize('adj_pr', [True, False])
    def test_get_data_null_as_missing_data(self, adj_pr):
-        #workaround as skip_on_exception decorator changes signature


Does using a fixture fix this?

@pytest.fixture(params=[True,False], scope='module') def adj_pr(request) return request.param @skip_on_exception(RemoteDataError) def test_get_data_null_as_missing_data(self, adj_pr): ...

@bashtage thanks for the code sample interesting usage of fixtures, wasn't able to run it successfully, but I am no pytest expert just a casual user. This approach has an additional level of indirection(a new fixture) and probably requires some advanced pytest knowledge, but a signature-preserving decorator hopefully should work just like any other decorator, just put one line in front of a function

I think @bashtage gave you everything there, so there is nothing else to do but copy and paste. What happens when you do this?

gfyoung · 2017-07-05T17:43:26Z

pandas_datareader/tests/yahoo/test_yahoo.py

+    @pytest.mark.parametrize('adj_pr', [True, False])
+    def test_get_data_null_as_missing_data(self, adj_pr):
+        # workaround as skip_on_exception decorator changes signature
+        # should it be changed to a signature-preserving decorator?


Decorators generally are difficult to make signature-preserving AFAIK, which is why I didn't bother to try when I created it 😄 . Does pytest and this decorator not work well together? Did you try using skip_on_exception above the pytest.mark one?

Yes, it's true that signature-preserving decorators are difficult to make but there are at least two packages that I was planning on trying.

I have tried all combinations of skip_on_exception'and the pytest.mark above and below.

What happens when you try either? (i.e. what stacktrace do you get?)

bellow is the error message

E TypeError: test_null_as_missing_data() takes exactly 2 arguments (1 given)

maybe it will work with global functions not when they are inside the class, not sure

I have looked at the decorator package not sure how to use it in our case, but there is another one that I can use too, can create an issue

yep no hurry at all.

I have looked at the decorator package not sure how to use it in our case,

IMO, using another package seems a bit overkill. I imagine that the fix for this issue shouldn't need to require another package just for this one decorator.

sorry, I didn't make it clear but it meant instead of a decorator package, one or the other, not an extra one in addition to the decorator package

I'm not sure that actually clarified what you were saying...but no worries, so long as the PR gets emerged eventually 😄

gfyoung · 2017-07-05T18:40:36Z

pandas_datareader/tests/yahoo/test_yahoo.py

@@ -134,7 +134,7 @@ def test_get_data_multiple_symbols(self):
    @pytest.mark.parametrize('adj_pr', [True, False])
    def test_get_data_null_as_missing_data(self, adj_pr):
        # workaround as skip_on_exception decorator changes signature
-        # should it be changed to a signature-preserving decorator?
+        # TODO: should it be changed to a signature-preserving decorator?


Given that signature preservation is difficult, I think a better message would be this:

def test_.. # TODO: We can't decorate the test function on top of # parametrization because it errors. Investigate this later.

i have tried both ways on top and below

def test_.. # TODO: We can't decorate the test function along with # parametrization because it errors. Investigate this later.

done, thanks; it's definitely better comments

bashtage · 2017-07-05T19:29:30Z

FWIW, this works as expected

Outside the class

@pytest.fixture(params=[True, False])
def adj_pr(request):
    return request.param

And the test function -- obviously one would remove the intentional raise RemoteDataError('This is a test')

    @skip_on_exception(RemoteDataError)
    def test_get_data_null_as_missing_data(self, adj_pr):
        result = web.get_data_yahoo('SRCE', '20160626', '20160705',
                                    adjust_price=adj_pr)
        # sanity checking
        assert result.dtypes.all() == np.floating
        raise RemoteDataError('This is a test')

jreback · 2017-07-05T22:49:33Z

@OlegShteynbuk if you want to change to use a fixture as @bashtage suggest would be great. ping on green.

OlegShteynbuk · 2017-07-05T23:52:04Z

I have tried @bashtage suggestion before with the fixture outside the class and inside the class, not sure what was the problem, maybe my environment is different, I have python 2.7.13 and pytest 3.0.7 on LInux, not sure what was his testing environment, maybe python 3; need to take a break...

It could be an interesting psychological experiment to find out if a preference for the solution depends on one's background; for me coming from Java and C++ encapsulation is a big deal and having a nested function makes it all nicely encapsulated in one place instead of having two functions in two different places; but it's always a good idea to look at the same problem from a different point of view, I already learned a lot about pytest from this discussion, thanks.

gfyoung · 2017-07-06T00:03:39Z

I think in this case I would go with what @bashtage implemented. Test fixtures are meant to be used across tests, so it makes sense to share this resource and not encapsulate. We have plenty of instances of encapsulation (functions within functions) in the codebase though.

Also, I'm a little surprised you went for inner functions given your Java and C++ background, since IINM, neither supports explicit declarations of inner functions. Sure, you have lambdas, but they are anonymous functions instead of the explicit one that you defined currently.

OlegShteynbuk · 2017-07-06T01:40:29Z

I have repeated the @bashtage test with pretty much the same results, doesn't run in my environment, below is the output and i have run it beforein different combinations, there are actually two problems as it says collected 21 items but it should be 22, and if you look at the output there is only one record for test_get_data_null_as_missing_data instead of two for True and False
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_null_as_missing_data <- pandas_datareader/_testing.py FAILED

below is the output

========================================================================

platform linux2 -- Python 2.7.13, pytest-3.0.7, py-1.4.31, pluggy-0.4.0 -- /home/oleg/programs/python/anaconda2/envs/pandas-datareader_dev/bin/python
cachedir: .cache
rootdir: /home/oleg/projects/openSource/python/pandas-datareader/pandas-datareader, inifile:
plugins: xdist-1.16.0
collected 21 items

pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_yahoo <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_multiple_symbols <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_adjust_price <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_multiple_symbols_two_dates <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_single_symbol <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_yahoo_DataReader_multi <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_yahoo_reader_class <- pandas_datareader/_testing.py PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_null_as_missing_data <- pandas_datareader/_testing.py FAILED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_yahoo_fails PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_quote_series PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_quote_string PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_quote_stringlist PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_quote_comma_name PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_components_dow_jones SKIPPED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_components_dax SKIPPED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_components_nasdaq_100 SKIPPED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_interval xfail
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_date_ret_index XPASS
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_yahoo_actions xfail
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_get_data_yahoo_actions_invalid_symbol PASSED
pandas_datareader/tests/yahoo/test_yahoo.py::TestYahoo::test_yahoo_DataReader xfail

==================================================================================================== FAILURES ====================================================================================================
__________________________________________________________________________________ TestYahoo.test_get_data_null_as_missing_data __________________________________________________________________________________

args = (<pandas_datareader.tests.yahoo.test_yahoo.TestYahoo object at 0x7f03d1549890>,), kwargs = {}

@wraps(f)
def wrapper(*args, **kwargs):
    try:

      f(*args, **kwargs)

E TypeError: test_get_data_null_as_missing_data() takes exactly 2 arguments (1 given)

pandas_datareader/_testing.py:26: TypeError
===================================================================== 1 failed, 13 passed, 3 skipped, 3 xfailed, 1 xpassed in 41.76 seconds ======================================================================

OlegShteynbuk · 2017-07-06T03:41:25Z

Also, I'm a little surprised you went for inner functions given your Java and C++ background, since IINM, neither supports explicit declarations of inner functions. Sure, you have lambdas, but they are anonymous functions instead of the explicit one that you defined currently.
=======================

there are also local and anonymous classes in Java that can be used to define a member function.
Was thinking more about the programming style that I was using most of the time in Java/C++, and also looked at this task as a standalone task not intended for reuse.

OlegShteynbuk · 2017-07-06T03:42:39Z

on a different note, probably a newbie github question, what does ping on green means, don't see any green buttons available for me to push on.

gfyoung · 2017-07-06T03:44:43Z

there are also local and anonymous classes in Java that can be used to define a member function

But they're anonymous nonetheless 😄

What @jreback means by pinging on green is this: when all of the tests pass (as they have now), just write another comment like this:

@jreback : Everything is green and ready to go!

OlegShteynbuk · 2017-07-06T03:54:39Z

@jreback : Everything is green and ready to go!

bashtage · 2017-07-06T08:55:10Z

@OlegShteynbuk is right -- Python 2.7 doesn't correctly preserve the function signature when decorated, and so pytest doesn't realize that it needs pass the fixture in. This results in the wrong number of arguments being passed, and an error occurs.

Here it fails (2.7)

https://travis-ci.org/bashtage/pandas-datareader/builds/250677402

While it works fine on all Python 3.x builds. Yet another reason for 2.7 pass to oblivion.

jreback · 2017-07-06T09:57:54Z

thanks!

pls open an issue to change the decorator as well.

OlegShteynbuk · 2017-07-06T12:55:53Z

issue #363

xref pydata#357

* TST: fix testing for yahoo get_data_null_as_missing comparisons xref #357 * Revert "CI: use latest pandas" This reverts commit 3b8b11a.

jreback requested changes Jul 3, 2017

View reviewed changes

OlegShteynbuk mentioned this pull request Jul 3, 2017

Exception downloading yahoo historical data with adjust_price=True on fix-yahoo branch #342

Closed

OlegShteynbuk changed the title ~~add 'null' in lower case to na_values argument to read_csv~~ fix 'null' in lower case as a missing value in a new Yahoo API Jul 3, 2017

OlegShteynbuk changed the title ~~fix 'null' in lower case as a missing value in a new Yahoo API~~ fix 'null' in lower case as a missing value in the new Yahoo API Jul 3, 2017

jreback requested changes Jul 4, 2017

View reviewed changes

jreback added this to the 0.5.0 milestone Jul 4, 2017

jreback added the bug label Jul 4, 2017

OlegShteynbuk added 10 commits July 5, 2017 09:00

add 'null' in lower case to na_values argument to read_csv

9ba7afb

fixed flake8 message that line is too long

db3d63e

fixed flake8 message

868105d

added test test_get_data_null_as_missing_data()

4a127f4

added a note in the whatsnew for 0.5

36844a7

added test to assert the dtypes of the result

f6f2b20

flake8

2b78a07

added test decorator

5313859

remove missleading comment

1cca12e

changed to use parametrize

ab1bcf7

OlegShteynbuk force-pushed the null_lower_case branch from d5ed549 to ab1bcf7 Compare July 5, 2017 13:14

flake8

225b1f3

bashtage reviewed Jul 5, 2017

View reviewed changes

OlegShteynbuk changed the title ~~fix 'null' in lower case as a missing value in the new Yahoo API~~ fix 'null' in lower case as a missing value in the new Yahoo API, make compatible with the older pandas versions Jul 5, 2017

gfyoung reviewed Jul 5, 2017

View reviewed changes

added TODO:

63a0a53

gfyoung reviewed Jul 5, 2017

View reviewed changes

improved comments as suggested by @gfyoung

aee54ba

jreback approved these changes Jul 5, 2017

View reviewed changes

jreback merged commit cdd8b4d into pydata:master Jul 6, 2017

OlegShteynbuk deleted the null_lower_case branch July 6, 2017 12:24

OlegShteynbuk mentioned this pull request Jul 6, 2017

Make "skip_on_exception" a signature-preserving decorator. #363

Closed

jreback added a commit to jreback/pandas-datareader that referenced this pull request Jul 10, 2017

TST: fix testing for yahoo get_data_null_as_missing comparisons

5d462bf

xref pydata#357

jreback mentioned this pull request Jul 10, 2017

TST: fix testing for yahoo get_data_null_as_missing comparisons #367

Merged

jreback added a commit to jreback/pandas-datareader that referenced this pull request Jul 10, 2017

TST: fix testing for yahoo get_data_null_as_missing comparisons

d21bdcc

xref pydata#357

jreback added a commit that referenced this pull request Jul 10, 2017

TST: fix testing for yahoo get_data_null_as_missing comparisons (#367)

a582b2b

* TST: fix testing for yahoo get_data_null_as_missing comparisons xref #357 * Revert "CI: use latest pandas" This reverts commit 3b8b11a.

fix 'null' in lower case as a missing value in the new Yahoo API, make compatible with the older pandas versions #357

fix 'null' in lower case as a missing value in the new Yahoo API, make compatible with the older pandas versions #357

Conversation

OlegShteynbuk commented Jul 3, 2017 • edited by jreback Loading

jreback commented Jul 3, 2017 • edited Loading

OlegShteynbuk commented Jul 3, 2017

OlegShteynbuk commented Jul 3, 2017 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegShteynbuk commented Jul 3, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jul 4, 2017

OlegShteynbuk commented Jul 5, 2017

Choose a reason for hiding this comment

OlegShteynbuk Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegShteynbuk Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegShteynbuk Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashtage commented Jul 5, 2017

jreback commented Jul 5, 2017

OlegShteynbuk commented Jul 5, 2017 • edited Loading

gfyoung commented Jul 6, 2017 • edited Loading

OlegShteynbuk commented Jul 6, 2017

OlegShteynbuk commented Jul 6, 2017

OlegShteynbuk commented Jul 6, 2017

gfyoung commented Jul 6, 2017

OlegShteynbuk commented Jul 6, 2017

bashtage commented Jul 6, 2017

jreback commented Jul 6, 2017

OlegShteynbuk commented Jul 6, 2017

OlegShteynbuk commented Jul 3, 2017 •

edited by jreback

Loading

jreback commented Jul 3, 2017 •

edited

Loading

OlegShteynbuk commented Jul 3, 2017 •

edited

Loading

OlegShteynbuk Jul 5, 2017 •

edited

Loading

gfyoung Jul 5, 2017 •

edited

Loading

OlegShteynbuk Jul 5, 2017 •

edited

Loading

OlegShteynbuk Jul 5, 2017 •

edited

Loading

gfyoung Jul 5, 2017 •

edited

Loading

OlegShteynbuk commented Jul 5, 2017 •

edited

Loading

gfyoung commented Jul 6, 2017 •

edited

Loading