Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Finance Options signatures and deprecate year/month parameters #3822

Merged
merged 4 commits into from
Jun 22, 2013
Merged

Change Finance Options signatures and deprecate year/month parameters #3822

merged 4 commits into from
Jun 22, 2013

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Jun 9, 2013

@gliptak
Copy link
Contributor Author

gliptak commented Jun 11, 2013

This is the error this build gets (my local unit test was running without errors ...). Investigating:

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

ERROR: test_options (pandas.io.tests.test_yahoo.TestYahoo)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.11.1.dev_d26f4b6-py2.7-linux-x86_64.egg/pandas/io/tests/test_yahoo.py", line 131, in test_options

    (calls, puts) = aapl.get_near_stock_price(call=True, put=True, dt=dt)

  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.11.1.dev_d26f4b6-py2.7-linux-x86_64.egg/pandas/io/data.py", line 768, in get_near_stock_price

    start_index = np.where(df_c['Strike'] > price)[0][0]

IndexError: index out of bounds

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

ERROR: test_options_warnings (pandas.io.tests.test_yahoo.TestYahoo)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.11.1.dev_d26f4b6-py2.7-linux-x86_64.egg/pandas/io/tests/test_yahoo.py", line 155, in test_options_warnings

    (calls, puts) = aapl.get_near_stock_price(call=True, put=True, month=month, year=year)

  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/pandas-0.11.1.dev_d26f4b6-py2.7-linux-x86_64.egg/pandas/io/data.py", line 768, in get_near_stock_price

    start_index = np.where(df_c['Strike'] > price)[0][0]

IndexError: index out of bounds

----------------------------------------------------------------------

Ran 3263 tests in 85.929s

FAILED (SKIP=42, errors=2)

@gliptak
Copy link
Contributor Author

gliptak commented Jun 11, 2013

Why wouldn't lxml.html be available under Python 2.6?

======================================================================
ERROR: test_options_warnings (pandas.io.tests.test_yahoo.TestYahoo)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.6_with_system_site_packages/lib/python2.6/site-packages/pandas-0.11.1.dev_23952c9-py2.6-linux-x86_64.egg/pandas/io/tests/test_yahoo.py", line 185, in test_options_warnings
    (calls, puts) = aapl.get_options_data(month=month, year=year)
  File "/home/travis/virtualenv/python2.6_with_system_site_packages/lib/python2.6/site-packages/pandas-0.11.1.dev_23952c9-py2.6-linux-x86_64.egg/pandas/io/data.py", line 526, in get_options_data
    from lxml.html import parse
ImportError: No module named lxml.html

@gliptak
Copy link
Contributor Author

gliptak commented Jun 12, 2013

Could somebody offer some comments on this? I cannot reproduce the same error locally running with/without network connectivity.

s = "Failed to download:\n{0}".format(e)
print s
return None
lines = urllib2.urlopen(urlStr).readlines()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason you are taking out this exception? This will catch a failed network download

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason to take out the exception

  • this will make it consistent with the behaviour of the other functions (which do throw this network error)
  • this allows in the unit test to mark the test "skip"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok...makes sense, to keep in mind, I raised this with @hayd on #3856, should replace this call with io.common._url_open or something (which can have options to handle e.g. gzip data), and also to have consistent exceptions, say NetworkError or somthing....fine for now, this is really a FYI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback I would like to make this work correctly first :)

@jreback
Copy link
Contributor

jreback commented Jun 13, 2013

@gliptak can you rebase to a smaller num of commits?

e.g

git rebase -i <your starting commit>
then f or s

@cpcloud
Copy link
Member

cpcloud commented Jun 13, 2013

git rebase -i upstream/master is nice too if you've already fetched and rebased. then u don't have to use commit hashes to remember where you started

@hayd
Copy link
Contributor

hayd commented Jun 13, 2013

That's a nice tip! I also recommend squashing before doing a rebase (that way you do fewer steps / one step for conflicts, and more likely to understand how they can be resolved).

@jreback
Copy link
Contributor

jreback commented Jun 13, 2013

@cpcloud maybe also add these tips to developer page? and It hink should have a link to developer page from somewhere in main docs......

@cpcloud
Copy link
Member

cpcloud commented Jun 13, 2013

there's a link...dev workflow is on my pandas to-do list: 2nd after eval!

@hayd
Copy link
Contributor

hayd commented Jun 13, 2013

#3156 :)

@gliptak
Copy link
Contributor Author

gliptak commented Jun 13, 2013

@jreback I did squash several commits. Thanks

@jtratner
Copy link
Contributor

@gliptak Right now the test cases for network errors aren't really doing what you want, because they catch all exceptions for bad tests, as opposed to specifically describing the behavior you want (e.g., should get_quote_yahoo bubble up a network error, or should it replace it with its own error?)

Catching exceptions with the baseclass Exception (and raising exceptions that are just Exception) is not great, because it means that users of the library have to catch bare Exceptions, as opposed to just opting into the exceptions they care about (i.e., you want to be able to see that you misspelled something as opposed to hitting a NetworkError).

Also, I suggest you consider changing the raise Exception(...) in io/data.py to raise a specific exception, i.e.:

from pandas.core.common import PandasError
class DataError(PandasError):
    pass

And that way you can catch these in your test cases, etc.

@gliptak
Copy link
Contributor Author

gliptak commented Jun 15, 2013

@jtratner note that in the code I'm not raising exceptions, although not catching/converting them to 'known' exceptions either. I believe the test code works, as it only marks the failing test skip if it notes a network problem (by trying to connect to google.com) The change you are suggesting is not related to this enhancement, I would be happy to work modifying current code as a separate pull request.

the reason that I widened to catching IOError in the tests, is that I have seen other errors beside URLError showing up in my testing

as per http://stackoverflow.com/a/3465796/304690 urllib2.urlopen throws HTTPError, URLError and ValueError

@jtratner
Copy link
Contributor

I mean exceptions like this:

    raise Exception("after %d tries, Yahoo did not "
                    "return a 200 for url %s" % (pause, url))

And in test cases, when it tests assertRaises(Exception, ... it makes it much less clear what behavior to expect.

@gliptak
Copy link
Contributor Author

gliptak commented Jun 15, 2013

@jtratner Jeff, that is existing code, unmodified from current HEAD. As I said above, i would be happy to continue refactoring under a new pull request.

@@ -430,8 +425,9 @@ def _parse_options_data(table):


class Options(object):
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move this import statement outside of the class definition (preferably at the top of the file). Right now Options has an attribute warnings which is this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jreback
Copy link
Contributor

jreback commented Jun 18, 2013

@gliptak this needs release notes / mention in v0.11.1

@gliptak
Copy link
Contributor Author

gliptak commented Jun 18, 2013

@jreback updated RELEASE.rst

@hayd
Copy link
Contributor

hayd commented Jun 19, 2013

@gliptak You're going to need to rebase again (it happens pretty often with the RELEASE.rst as everyone is adding to it)

Tip: don't add your release note at the end, that way it's less likely to conflict :)

@gliptak
Copy link
Contributor Author

gliptak commented Jun 19, 2013

@hayd Thanks Andy. Moved my note higher up.

@hayd
Copy link
Contributor

hayd commented Jun 19, 2013

(There's no guarantee it will merge cleanly even when not at the bottom, (I claim) it just makes it less lightly - so there still could be a conflict even when moved, only way to be sure is to do the rebase :) )

@jtratner
Copy link
Contributor

@gliptak can you edit this after the new decorators are merged in and remove the in-test IOError checks?

@gliptak
Copy link
Contributor Author

gliptak commented Jun 19, 2013

@jtratner I can work that tomorrow afternoon/evening.

@cpcloud
Copy link
Member

cpcloud commented Jun 21, 2013

@gliptak looks like you tried to merge from upstream. i'm going to pull down ur branch and take a look

@cpcloud
Copy link
Member

cpcloud commented Jun 21, 2013

i used to do this until i understood merge vs. rebase. going forward it's key that u understand the differences so that your contributions will be merged faster...

@cpcloud
Copy link
Member

cpcloud commented Jun 21, 2013

ok so all you need to do is the following

git fetch upstream
git rebase upstream/master
# resolve the merge conflict generated here
git add pandas/io/data.py
git rebase --continue

@cpcloud
Copy link
Member

cpcloud commented Jun 21, 2013

i use the first 2 commands so often that i have them aliased as

alias gfrb='git fetch upstream && git rebase upstream/master'

@jreback
Copy link
Contributor

jreback commented Jun 21, 2013

@cpcloud fyi.....should put how to rebase on the wiki (I think you put on developer pages as well?)

@gliptak
Copy link
Contributor Author

gliptak commented Jun 22, 2013

@jreback this is what i basically did, but i could not complete the rebase ...

@cpcloud
Copy link
Member

cpcloud commented Jun 22, 2013

what do you mean you couldn't complete it?

@cpcloud
Copy link
Member

cpcloud commented Jun 22, 2013

@jreback anything elseyou want me to add to the wiki?

@jreback
Copy link
Contributor

jreback commented Jun 22, 2013

looks good...maybe a bit more about how to actually run git rebase -i <commit>.....e.g. what/how to squash

@gliptak
Copy link
Contributor Author

gliptak commented Jun 22, 2013

@cpcloud I got:
git fatal: unrecognized input Repository lacks necessary blobs to fall back on 3-way merge.
basically ended up hand merging changes

@cpcloud
Copy link
Member

cpcloud commented Jun 22, 2013

hm i only get git fatals when a lock file isn't removed...then usually it is when i check and then i run it again...if u get stuck on a rebase do git rebase --abort and try again to see .e.,g if it was a lock problem

@gliptak
Copy link
Contributor Author

gliptak commented Jun 22, 2013

@cpcloud yes, I tried. even after git rebase --abort, still seen the same message. But now, I hope I'm back in business.

@cpcloud
Copy link
Member

cpcloud commented Jun 22, 2013

hm i pulled down ur branch just after your first message and things went except for 2 merge conflicts which were easy to resolve...

@@ -897,11 +876,21 @@ def get_near_stock_price(self, above_below=2, call=True, put=False,
else:
return chop_put

def _try_parse_dates(self, year, month, expiry):
if (year is not None or month is not None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to change this but fyi there's no need for the parens, they are implicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@jreback
Copy link
Contributor

jreback commented Jun 22, 2013

@gliptak looks ready to go?

@gliptak
Copy link
Contributor Author

gliptak commented Jun 22, 2013

@jreback I think so

jreback added a commit that referenced this pull request Jun 22, 2013
Change Finance Options signatures and deprecate year/month parameters
@jreback jreback merged commit 2b4fd7c into pandas-dev:master Jun 22, 2013
@jreback
Copy link
Contributor

jreback commented Jun 22, 2013

thank you

@jreback
Copy link
Contributor

jreback commented Jun 27, 2013

can you have a look? test_options keeps failing sometimes..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants