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

Added basic inequality mesasures: lorenz curve and gini #414

Merged
merged 3 commits into from
Oct 11, 2018

Conversation

cdagnino
Copy link
Contributor

@cdagnino cdagnino commented Jun 4, 2018

First stab at creating inequality indices, as discussed in #395

Tests added and nosetests passed.

@coveralls
Copy link

coveralls commented Jun 4, 2018

Coverage Status

Coverage decreased (-0.8%) to 94.405% when pulling 176801b on cdagnino:inequality_measures into 8657c91 on QuantEcon:master.

@jstac
Copy link
Contributor

jstac commented Jun 5, 2018

Thanks @cdagnino

For me, prange is not working once you wrap computation of the Gini coefficient in a jitclass. I can see that by monitoring system resources.

Since jitclass is still experimental and seems to conflict with prange, I recommend that we keep computation of the Gini coefficient as a function with @njit(parallel=True) as a decorator.

I can see your motivation for wrapping the functionality in a class, since you propose additional methods, but the cost of preserving these operations as individual functions is not high.

@mmcky
Copy link
Contributor

mmcky commented Jun 5, 2018

thanks @cdagnino this is looking good. I agree with @jstac comment re jitclass.

In other code I often write jit functions that do a lot of heavy lifting which makes them accessible (through import) at the function level, and when there is a use case I often use those fast functions to construct a convenience class that holds data and methods and in turn make calls the collection of general functions.

@jstac
Copy link
Contributor

jstac commented Jul 23, 2018

@cdagnino Thanks for your help to date. Should we leave this to you or do you not have time?

@cdagnino
Copy link
Contributor Author

@jstac @mmcky Apologies for the (really) long silence. I've pushed the changes you suggested. Let me know if I should do a pull request now

Copy link

@rebekahanne rebekahanne left a comment

Choose a reason for hiding this comment

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

@cdagnino This looks good. I just made a few comments that might help the documentation be a bit cleaner.

quantecon/inequality.py Outdated Show resolved Hide resolved
quantecon/tests/test_inequality.py Outdated Show resolved Hide resolved
@mmcky
Copy link
Contributor

mmcky commented Oct 6, 2018

thanks @cdagnino and @rebekahanne -- look forward to merging this.

@cdagnino
Copy link
Contributor Author

cdagnino commented Oct 6, 2018

@mmcky @rebekahanne I pushed the suggested changes to the docs. Let me know if anything else is missing :-D

@mmcky mmcky added the ready label Oct 10, 2018
@mmcky
Copy link
Contributor

mmcky commented Oct 10, 2018

thanks @cdagnino and @rebekahanne. I have tagged this as ready and it will be merged later today.

@mmcky mmcky merged commit b52559f into QuantEcon:master Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants