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

Vectorize lorenz_curve and gini #702

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Smit-create
Copy link
Member

@Smit-create Smit-create commented Apr 19, 2023

  • Replaced the python loops with vectorized code for enhancing the performance in lorenz_curve and gini_coefficient.

@coveralls
Copy link

coveralls commented Apr 19, 2023

Coverage Status

Coverage: 92.988% (+0.01%) from 92.975% when pulling c448868 on Smit-create:opt into eeb0865 on QuantEcon:main.

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

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

Maybe add tests?

@Smit-create
Copy link
Member Author

Hmm, The tests are already present in https://github.com/QuantEcon/QuantEcon.py/blob/main/quantecon/tests/test_inequality.py so I thought to skip adding them.

@oyamad
Copy link
Member

oyamad commented Apr 23, 2023

Is this adding new features or refactoring the code?

@Smit-create
Copy link
Member Author

This is just refactoring the code and removing some for loops for performance optimizations.

@oyamad
Copy link
Member

oyamad commented Apr 24, 2023

@Smit-create It is usually advised to use for loops (and avoid creation of intermediate arrays) in njited functions. Do you have a reason to go against that in this case?

@Smit-create
Copy link
Member Author

It is usually advised to use for loops (and avoid creation of intermediate arrays) in njited functions.

I found this better in terms of time performance as this is vectorized version while the extra space complexity (creating new arrays) is still the same for both the functions -- O(n)

@mmcky
Copy link
Contributor

mmcky commented Apr 27, 2023

@Smit-create thanks for opening this PR.

Would you mind to update the top level comment box with a description of the change and document the performance improvements you mention in this thread so it's clear what this change is about.

@Smit-create
Copy link
Member Author

Thanks @mmcky. Done

@mmcky
Copy link
Contributor

mmcky commented Oct 24, 2023

@Smit-create I haven't merged this as it isn't clear what level of performance improvement is being made. Is it 2% or 20% for example, it would be helpful to have timings between the different versions to make an assessment here. @oyamad makes a good point around njit so we need a clear reason to change to vectorized code.

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

Successfully merging this pull request may close these issues.

4 participants