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

use pd.Int64Index() #158

Merged
merged 1 commit into from Mar 14, 2016
Merged

use pd.Int64Index() #158

merged 1 commit into from Mar 14, 2016

Conversation

juanshishido
Copy link
Contributor

  • return pd.DataFrame with pd.Int64Index() index in remove_rows()
  • add assert_empty_int64index() to use with test_tgrtransition_with_accounting()

I tried changing as little code as possible.

Note that the failed build is due to changes in pandas (possibly 0.18 (or >=0.17.1)), as commented here.

Alternatively, for the test_*remove*all() tests at least, the check_index_type option in pdt.assert_frame_equal() can be set to False. This isn't guaranteed to fix everything.

* return pd.DataFrame with pd.Int64Index() index in `remove_rows()`
* add `assert_empty_int64index() to use with `test_tgrtransition_with_accounting()`
@juanshishido
Copy link
Contributor Author

New error that I hadn't seen previously related to a TypeError in urbansim/developer/tests/test_developer.py.

waddell added a commit that referenced this pull request Mar 14, 2016
@waddell waddell merged commit e84c856 into UDST:master Mar 14, 2016
@fscottfoti
Copy link
Contributor

So this new error is weird, right? Seems like it's internal to numpy's round function? Is it easy to recreate?

@bridwell
Copy link
Contributor

Per the prior issue: do we want to assume that the index will always be an int64? Could we instead just do:

remove_rows.index = pd.Index(remove_rows.index.values, data.dtype)

Also, do you know what pandas method is causing the index type to change? If the change happens in the sample_rows method (that method has a call to pd.concat that seems like it could be the culprit) then perhaps the change should be pushed there?

@juanshishido
Copy link
Contributor Author

@fscottfoti Recreated using np.round(pd.Series(range(10))) with pandas==0.18.0. No error with 0.17.1.

@bridwell The only thing I might guess could be causing the index type to change is this.

This and #159 are kind of hacky. It sounds like you want uniformity and consistency in the indices across objects, which I 100% agree with. The changes I made were meant to touch as little code as possible, but there may be inconsistencies with some pd.Int64Index and some pd.Index. This would require a deeper dive through the codebase, though.

Also, issues like these arise because of http://repo.continuum.io/miniconda/Miniconda-latest-Linux-x86_64.sh in .travis.yml. I'm not too familiar with Anaconda, but can we specify specific versions of packages? This could help mitigate problems due to changes in third party packages.

@jiffyclub
Copy link
Member

You can specify the library versions (which you would do here), but I generally like to notified of these kind of changes ASAP instead of finding out about them from users later. These issues are minor, but if things get harrier you may need to set up Travis to test against multiple versions of Pandas.

@juanshishido
Copy link
Contributor Author

Good point, @jiffyclub! Thanks for the input.

I would defer to the maintainers (i.e., the individuals that are part of UDST) on how best to proceed. We could, for example, revert the changes I submitted and do something like what @bridwell suggested. Just let me know how I can help.

@fscottfoti
Copy link
Contributor

Right, it seems like it might break code that has a non-integer index, unfortunately. My opinion is that a difference in a 32 bit integer index and 64 bit integer index is immaterial from UrbanSim's perspective. I blame the tests and might just force the type to 32 bit there if that's easy.

@jiffyclub
Copy link
Member

I agree that hard-coding the index type in the code is not ideal. Why didn't using check_index_type=False with pdt.assert_frame_equal work?

@juanshishido
Copy link
Contributor Author

@jiffyclub that absolutely did work and I listed it as an alternative to what I submitted.

Shall I revert and make that change? (This won't cover the np.round() issue, though.) cc: @fscottfoti

juanshishido added a commit to juanshishido/urbansim that referenced this pull request Mar 14, 2016
This reverts commit e84c856, reversing
changes made to 058a930.
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.

None yet

5 participants