Skip to content

Conversation

@verstatx
Copy link
Contributor

@verstatx verstatx commented Oct 4, 2023

Description

Adds signed 8-bit integer support to all arrayfire backends. So far all backends build and most tests pass. Failing tests:

  • AnisotropicDiffusion/4.GradientGrayscale
  • AnisotropicDiffusion/4.GradientColorImage
  • AnisotropicDiffusion/4.CurvatureGrayscale
  • AnisotropicDiffusion/4.CurvatureColorImage
  • InverseDeconvolution/1.TikhonovOnGrayscale
  • IterativeDeconvolution/1.LandweberOnGrayscale
  • IterativeDeconvolution/1.RichardsonLucyOnGrayscale (skipped)
    RichardsonLucy doesn't work well with negative values. Test passes when data is 0-127.
  • RotateLinear/6.* (skipped)
    Test data for every non-cardinal rotation doesn't work well with s8.
  • TransformInt/6.PerspectiveBilinear
  • TransformInt/6.PerspectiveBilinearInvert

All of the image loading tests appear to be failing due to unsigned->signed conversion overflow related to image loading. (fixed)

Changes to Users

No additional build flags have been added for this feature.

Checklist

  • Rebased on latest master
  • Code compiles
  • Tests pass (skipping RichardsonLucyOnGrayscale and RotateLinear tests)
  • Functions documented

@umar456
Copy link
Member

umar456 commented Oct 5, 2023

Hey, things look good but I am getting the same failures you are. Do you need us to fix these or do you think you can handle this?

@verstatx
Copy link
Contributor Author

verstatx commented Oct 6, 2023

I am looking into it, but if you could look into the remaining failing tests, that would be very much appreciated! The last 3 are all related to bilinear interpolation, but I'm not sure about the RichardsonLucyOnGrayscale test.

#if AF_API_VERSION >= 37
, f16 ///< 16-bit floating point value
#endif
, s8 ///< 8-bit signed integral value /// TODO AF_API_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
, s8 ///< 8-bit signed integral value /// TODO AF_API_VERSION
#if AF_APU_VERSION >= 310
, s8 ///< 8-bit signed integral value /// TODO AF_API_VERSION
#endif

Copy link
Contributor Author

@verstatx verstatx Oct 14, 2023

Choose a reason for hiding this comment

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

With the current system, if I use 310 API versions 4.0+ will need to be set to 400, not 40. This is not really a problem currently, but maybe needs to be looked at in the future?

Comment on lines 178 to 188

template<>
struct dtype_traits<signed char> {
enum {
af_type = s8 ,
ctype = f32
};
typedef signed char base_type;
static const char* getName() { return "schar"; }
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
template<>
struct dtype_traits<signed char> {
enum {
af_type = s8 ,
ctype = f32
};
typedef signed char base_type;
static const char* getName() { return "schar"; }
};
#if AF_API_VERSION >= 310
template<>
struct dtype_traits<signed char> {
enum {
af_type = s8 ,
ctype = f32
};
typedef signed char base_type;
static const char* getName() { return "schar"; }
};
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Public interface changes need to be surrounded with these ifdefs. This includes anything in the include folder but not in the src folder.

@verstatx verstatx force-pushed the int8 branch 4 times, most recently from 94e7674 to 70463f2 Compare October 16, 2023 20:38
@verstatx verstatx changed the title [WIP] signed 8-bit integer support signed 8-bit integer support Oct 23, 2023
AFAPI array operator&(const array& lhs, const long long& rhs);
AFAPI array operator&(const array& lhs, const long& rhs);
AFAPI array operator&(const array& lhs, const short& rhs);
AFAPI array operator&(const array& lhs, const signed char& rhs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do the bitwise and logical AND operators need to be protected by the ifdefs? None of the other types have guards.

@verstatx verstatx force-pushed the int8 branch 2 times, most recently from bbf3fc5 to 3017696 Compare November 5, 2023 23:29
@melonakos melonakos added this to the 3.10 milestone Feb 11, 2025
@melonakos melonakos marked this pull request as ready for review February 11, 2025 21:51
Image loading with a shift causes the test data to contain negative values.
This test passes when the data is limited to 0-127 instead.
This test data cannot be trivially shifted since rotate sets out-of-bounds
values to 0, not -128. Limiting the range to 0-127 mostly works, but introduces
rounding errors between output and gold.
This also slightly extends the interface macros.
Copy link
Contributor

@edwinsolisf edwinsolisf left a comment

Choose a reason for hiding this comment

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

Tested on all backends on RTX3070 Ti both on Windows and Ubuntu

@syurkevi syurkevi merged commit 7b82364 into arrayfire:master Mar 11, 2025
2 of 4 checks passed
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.

5 participants