Skip to content

BUG: Force npymath to respect npy_longdouble #20360

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

Merged
merged 2 commits into from
Nov 13, 2021

Conversation

seiko2plus
Copy link
Member

@seiko2plus seiko2plus commented Nov 12, 2021

closes #20348

In some certain cases mostly workarounds, there's a necessity to define npy_longdouble
as double even if that was against the compiler implementation(sizeof(long double) != sizeof(double)).

Therefore, avoid long double, use npy_longdouble instead,
and when it comes to standard math functions make sure of using
the double version when NPY_SIZEOF_LONGDOUBLE == NPY_SIZEOF_DOUBLE.

…t `npy_longdouble`

  In some certain cases mostly workarounds, there's a necessity to define `npy_longdouble`
  as `double` even if that was against the compiler
  implementation(sizeof(long double) != sizeof(double)).

  Therefore, avoid `long double`, use `npy_longdouble` instead,
  and when it comes to standard math functions make sure of using
  the double version when `NPY_SIZEOF_LONGDOUBLE` == `NPY_SIZEOF_DOUBLE`.
@seiko2plus seiko2plus changed the title BUG: Revert from long double changes, and force npymath to respet npy_longdouble BUG: Revert from long double changes, and force npymath to respect npy_longdouble Nov 12, 2021
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Nov 12, 2021
@rgommers rgommers added this to the 1.22.0 release milestone Nov 12, 2021
@rgommers
Copy link
Member

rgommers commented Nov 12, 2021

CI seems happy. Is this in Draft mode on purpose still @seiko2plus?

Cc @carlkl. .

@seiko2plus seiko2plus marked this pull request as ready for review November 12, 2021 20:05
@seiko2plus
Copy link
Member Author

seiko2plus commented Nov 12, 2021

@rgommers, complex long double needs a visit, still buggy if c99 complex is enabled and sizeof(npy_longdouble) != sizeof(long double). @carlkl, Have you tested complex operations on MinGW in runtime before?

@seiko2plus
Copy link
Member Author

seiko2plus commented Nov 12, 2021

it's fine to merge it, since the patch fixes the build on arm7 and m1 without changing the behavior of npy_longdouble.

@charris charris added 09 - Backport-Candidate PRs tagged should be backported and removed 09 - Backport-Candidate PRs tagged should be backported labels Nov 12, 2021
@carlkl
Copy link
Member

carlkl commented Nov 12, 2021

@seiko2plus, concerning complex long double in npy_common.h:

The defintion of npy_clongdouble in npy_common.h is fine as it is:

#if defined(NPY_USE_C99_COMPLEX) && defined(NPY_HAVE_COMPLEX_LONG_DOUBLE)
typedef complex long double npy_clongdouble;
#else
typedef struct {npy_longdouble real, imag;} npy_clongdouble;
#endif

as NPY_HAVE_COMPLEX_LONG_DOUBLE is not defined for MSVC numpy we get:

typedef struct {npy_longdouble real, imag;} npy_clongdouble;

which is correct in our case if (and only if) npy_longdoube is defined as double. BTW: I have tested this during my investigations with the scipy builds.

@seiko2plus
Copy link
Member Author

@carlkl, msvc is fine in both cases since sizeof(long double) always equal to sizeof(npy_longdouble), the issue would be in MinGW or any other platform where sizeof(long double) != sizeof(npy_longdouble) and the following path or any other similar C99 pathes are activated:

/**begin repeat1
* #kind = cexp,clog,csqrt,ccos,csin,ctan,ccosh,csinh,ctanh,
* cacos,casin,catan,cacosh,casinh,catanh#
* #KIND = CEXP,CLOG,CSQRT,CCOS,CSIN,CTAN,CCOSH,CSINH,CTANH,
* CACOS,CASIN,CATAN,CACOSH,CASINH,CATANH#
*/
#ifdef HAVE_@KIND@@C@
@ctype@
npy_@kind@@c@(@ctype@ z)
{
__@ctype@_to_c99_cast z1;
__@ctype@_to_c99_cast ret;
z1.npy_z = z;
ret.c99_z = @kind@@c@(z1.c99_z);
return ret.npy_z;
}
#endif

However, since we only do this workaround on MSVC & MinGW and both had been tested then I think we're fine.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Great, this looks good so let's get it in. Thanks @seiko2plus, @carlkl and everyone else who weighed in!

@rgommers rgommers merged commit 9ba1440 into numpy:main Nov 13, 2021
charris pushed a commit to charris/numpy that referenced this pull request Nov 25, 2021
…t `npy_longdouble` (numpy#20360)

In some certain cases mostly workarounds, there's a necessity to define `npy_longdouble`
as `double` even if that was against the compiler
implementation(sizeof(long double) != sizeof(double)).

Therefore, avoid `long double`, use `npy_longdouble` instead,
and when it comes to standard math functions make sure of using
the double version when `NPY_SIZEOF_LONGDOUBLE` == `NPY_SIZEOF_DOUBLE`.
@charris charris changed the title BUG: Revert from long double changes, and force npymath to respect npy_longdouble BUG: Force npymath to respect npy_longdouble Nov 25, 2021
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Nov 25, 2021
@charris charris removed this from the 1.21.5 release milestone Nov 25, 2021
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.

BUG: typedef long double npy_longdouble; creates problems on platforms with double == longdouble
4 participants