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

20161109 bugfix in models.transition.remove_rows #177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

moritz-kirmse-forcity
Copy link

Correction for the following problem:

error handling is too strict:
whith accounting_column != None, the function won't allow removing an amount len(data) < nrows < data[accounting_columns].sum(), even if this is possible.

Moritz Kirmse added 2 commits November 9, 2016 19:21
    condition: the total to be removed with an accounting column is higher than the total of agents

    modification of remove_rows function so that the function does not abort
must check if accounting column is given before accessing columns for sum
@moritz-kirmse-forcity
Copy link
Author

Checking why tests have failed.
Failed tests might be related to the random sampling in transition module. If the algorithm does not converge after 50 iterations an incorrect result could be returned depending on the random seed.

@moritz-kirmse-forcity
Copy link
Author

I found 2 issues with this PR:

  1. deprecated method Index.diff has been removed in pandas 0.19, breaking some of the urbansim code
  2. I had some failed tests where the sampling in remove_rows does not return the correct total with respect to accounting_column. -> opening an issue for that No feedback if iteration does not reach exact result in utils.sampling.sample_rows #178:

However, the bugfix remains valid even if the tests fail.

temporary workaround for problem with random permutation
@moritz-kirmse-forcity
Copy link
Author

moritz-kirmse-forcity commented Nov 10, 2016

updated the tests for transition, the tests pass on local system with pandas 0.18.1
There is still a workaround in line 154 due to misbehavior of the random permutation. This shall be updated when issue #178 is solved

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 94.179% when pulling fa1fcb7 on moritz-kirmse-forcity:20161109_fix_remove into caa6ab8 on UDST:master.

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.

2 participants