Conversation
There was a problem hiding this comment.
while here could we adopt the standard import numpy as np?
There was a problem hiding this comment.
great point! actually i did ran a sonaqube code analysis scanning which also pointed that, we could have a discussion about those issues all together for improvement https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&id=noman404_policyengine-core
There was a problem hiding this comment.
like there are several class methods do not have self also another standard practice
| @@ -0,0 +1,6 @@ | |||
| - bump: major | |||
There was a problem hiding this comment.
is this backwards-incompatible?
There was a problem hiding this comment.
Python codeabase should be fine, cause there is no change related to python language specific code. However numpy is not compatible, there are several changes in data type like, float64, int64, inf, default type definition etc.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #338 +/- ##
==========================================
- Coverage 84.73% 84.15% -0.59%
==========================================
Files 191 194 +3
Lines 9773 9989 +216
Branches 1018 1046 +28
==========================================
+ Hits 8281 8406 +125
- Misses 1204 1290 +86
- Partials 288 293 +5 ☔ View full report in Codecov by Sentry. |
|
That did strike me as strange in that part of the codebase, but I believe that’s actually by design, the idea being to obscure that part of the codebase away while building variables and other objects that proliferate throughout the tax code.
… On Jan 31, 2025, at 12:54 AM, Al Noman ***@***.***> wrote:
@noman404 commented on this pull request.
On policyengine_core/taxscales/marginal_rate_tax_scale.py <#338 (comment)>:
like there are several class methods do not have self also another standard practice
—
Reply to this email directly, view it on GitHub <#338 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSK7WZZQ4VKFNSB55ZL3632NK3TDAVCNFSM6AAAAABWDRRYYSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOBVGM2TCMRRGQ>.
You are receiving this because you are subscribed to this thread.
|
|
@MaxGhenis we are stuck in a circular dependency scenario here, the test test_us on 3.13 will fail because current PE-US does not use 3.13 and PE-US also fail as current PE-Core not 3.13 |
@nikhilwoodruff @anth-volk @mikesmit any tips? |
# Conflicts: # .github/workflows/pr.yaml
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Have a few questions on this and would love @nikhilwoodruff's review
| install: | ||
| pip install -e ".[dev]" --config-settings editable_mode=compat | ||
| pip install policyengine-us | ||
| pip install git+https://github.com/noman404/policyengine-us.git@noman404/python3.13 |
There was a problem hiding this comment.
issue, blocking: This ties us to an installation we do not want
There was a problem hiding this comment.
question: @nikhilwoodruff How did we resolve the overlapping deps issue Al raised before? And is it still present?
| if isinstance(this, tuple): | ||
| raise TypeError("First argument must not be a tuple.") | ||
|
|
||
| if isinstance(that, tuple): |
There was a problem hiding this comment.
nit, non-blocking: Can't we just check both of these args and have one message for both?
| # because of the infinities the first sort creates positional indices | ||
| # The second argsort converts these positions to ranks, thus fixes the broken sort issue | ||
| first_argsort = numpy.argsort(matrix, axis=1, kind="stable") | ||
| sorted_matrix = numpy.argsort(first_argsort, axis=1, kind="stable") |
There was a problem hiding this comment.
blocking: I'd love @nikhilwoodruff to confirm the logic here, as I'm less familiar with numpy
| array(['this1.0', 'that2.5']...) | ||
|
|
||
| """ | ||
| if isinstance(this, tuple): |
There was a problem hiding this comment.
question: Why can't these be tuples?
| "pyvis>=0.3.2", | ||
| "microdf_python>=0.4.3", | ||
| "huggingface_hub>=0.25.1", | ||
| "standard-imghdr", |
There was a problem hiding this comment.
question: Why do we need this package? I see no reference to it in the PR changes
| # numpy.finfo(float_).eps | ||
| thresholds1 = numpy.outer( | ||
| factor + numpy.finfo(numpy.float_).eps, | ||
| factor + numpy.finfo(numpy.float64).eps, |
There was a problem hiding this comment.
Documented at https://numpy.org/doc/2.1/numpy_2_0_migration_guide.html
|
|
||
| zone = np.asarray([TypesZone.z1, TypesZone.z2, TypesZone.z2, TypesZone.z1]) | ||
| zone = np.asarray( | ||
| [ |
There was a problem hiding this comment.
question: Why does this behavior change?
|
Superseded by #365 |
3 participants
Fixes #334