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

Fix Integer Overflow #286

Merged
merged 3 commits into from
Dec 27, 2019
Merged

Conversation

danielsclint
Copy link

@danielsclint danielsclint commented Dec 20, 2019

This address #285 to fix overflows when calculating the memory footprint for the skims. This replaces the Numpy.prod method with a straight Python integer multiplication to let Python handle the growth of the integer value under the hood.

This conversation on Stack Overflow provides a good synopsis of the problem and the solution.

Rather than replace Numpy prod in line with the multiplication, I created a short method to do multiplication that might result in big numbers. This understandably may not be the most elegant solution, but it does allow me to write a test around the problem.

def multiply_large_numbers(list_of_numbers):
    return reduce(mul, list_of_numbers)

Review Criteria Responses

  1. Does it contain all the required elements, including a runnable example, documentation, and tests?
    This code doesn't change the examples or documentation. It is a background fix to eradicate a bug. A test has been written to prove that it works.

  2. Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
    Yes. It allows for ActivitySim to support much larger implementations.

  3. Are the runtimes reasonable and does it provide documentation justifying this claim?
    This change has no material impact on runtimes.

  4. Does it include non-Python code, such as C/C++? If so, does it compile on any OS and are compilation instructions included?
    No. This is a Python-only change.

  5. Is it licensed with the ActivitySim license that allows the code to be freely distributed and modified and includes attribution so that the ‘provenance’ of the code can be tracked? Does it include an official release of ownership from the funding agency if applicable?
    This work was done under contract to ARC, and, presumably, ARC is providing the changes without any additional licensing beyond the existing ActivitySim licensing.

  6. Does it appropriately interact with the data pipeline (i.e. it doesn't create new ways of managing data)?
    This change does not impact the data pipeline.

  7. Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework?
    No regression testing has been done. The code change fix a bug that results in incorrect arithmetic in certain situations.

  8. Does it include sufficient test coverage and test data for existing and proposed features?
    Yes, a test was written along with scaffolding to test other methods in the skims.py file.

  9. Any other comments or suggestions for improving the developer experience?
    No

@coveralls
Copy link

coveralls commented Dec 20, 2019

Coverage Status

Coverage increased (+0.04%) to 86.189% when pulling 772cb34 on danielsclint:ft_overflow into 7b57c94 on ActivitySim:master.

@bstabler
Copy link
Contributor

@danielsclint - please rebase this against the develop branch so it incorporates #275

@danielsclint danielsclint changed the base branch from master to develop December 26, 2019 19:37
@danielsclint
Copy link
Author

I updated the target branch to develop.

@bstabler bstabler merged commit efc2bc6 into ActivitySim:develop Dec 27, 2019
@danielsclint danielsclint deleted the ft_overflow branch August 10, 2020 04:50
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.

3 participants