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

Changes for numpy 2.0 to fix test matrix. #1196

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

Thrameos
Copy link
Contributor

@Thrameos Thrameos commented Jun 17, 2024

This PR deals with the broken test matrix following the removal for float_. I replaced it with float16 though that is actually a different test. I dealt with the required alterations to support half type for array conversion, though I can't find a good pattern to deal with byte reversed arrays so that may be untested.

@Thrameos Thrameos requested a review from marscher June 17, 2024 04:49
@Thrameos Thrameos added the dependencies Pull requests that update a dependency file label Jun 17, 2024
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 63.82979% with 17 lines in your changes missing coverage. Please review.

Project coverage is 87.33%. Comparing base (cdb6be2) to head (f6849c5).
Report is 6 commits behind head on master.

Files Patch % Lines
native/common/jp_convert.cpp 63.82% 15 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
- Coverage   87.44%   87.33%   -0.12%     
==========================================
  Files         113      113              
  Lines       10238    10281      +43     
  Branches     4059     4065       +6     
==========================================
+ Hits         8953     8979      +26     
- Misses        691      706      +15     
- Partials      594      596       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Thrameos
Copy link
Contributor Author

Seem like I gave one more set of changes before this is complete. Not sure why onle one version triggered it.

@Thrameos
Copy link
Contributor Author

I am unable to replace the error seen in the test suite but it most likely is a Python behavior not a JPype one. The test was making sure that all cast types go through required paths. Python itself through the exception that "e" type is not a simple data type. If they choose to change that behavior then we shouldn't test against it.

@marscher I believe this is ready to go.

Copy link
Contributor Author

@Thrameos Thrameos left a comment

Choose a reason for hiding this comment

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

Not much to this other than changes to the test bench to get it back on line.

The only odd code was to better support the half float which is valid buffer type in Python. There is no native Java type for it.


namespace
{

template <jvalue func(void *c) >
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a crude implementation of the "half" floating type. There is no native type in Java to support this but it is possible that someone could pass a half floating in for promotion to either float or double. This code is exercised in the test suite.

@@ -385,6 +436,31 @@ jconverter getConverter(const char* from, int itemsize, const char* to)
case 'd': return &Convert<double>::toD;
}
break;
case 'e':
if (reverse) switch (to[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if there is every a case in which a reversed byte order half floating value would be converted. Through the order of this operation is reasonable to define. Reverse the bytes, promote to float then convert to Java type.

@marscher
Copy link
Member

Thanks. LGTM

@marscher marscher merged commit 5b934d7 into jpype-project:master Aug 19, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants