Skip to content

BUG: fix for evaluation of random_f and random_standard_cauchy in distributions.c #29598

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

BUG: fix for evaluation of random_f and random_standard_cauchy in distributions.c #29598

wants to merge 5 commits into from
+6 −3

Conversation

zuhu2195
Copy link

C program does not guarantee the order of evaluation of its operands, as a result if we have a function that returns f1()/f2(),
here f2() may get evaluated first. consider the following program,

#include <stdio.h>

/* Dummy bit generator state */
typedef struct {
    int call;
} bitgen_t;

/* Deterministic "random_standard_normal" function */
double random_standard_normal(bitgen_t *bitgen_state) {
    if (bitgen_state->call == 0) {
        bitgen_state->call++;
        return 8.0;   /* first fixed value */
    } else {
        return 2.0;   /* second fixed value */
    }
}

/* Function returning ratio of two standard normals */
double random_ratio(bitgen_t *bitgen_state) {
    return random_standard_normal(bitgen_state) /
           random_standard_normal(bitgen_state);
}

int main(void) {
    bitgen_t state = {0}; /* no randomness */

    double val = random_ratio(&state);
    printf("Deterministic ratio value: %f\n", val);

    return 0;
}

here the function random_standard_normal uses bitgen_state struct to evaluate the first and second call, but since C guarantees no order the first call might be f2() which can cause return of 8.0 in f2() and 2.0 in f1() clearly the result returned will be 2.0/8.0 which if 1/4, however if f1() evaluates first result returned will be 8.0/2.0 which is 4.

@zuhu2195
fix for errneous code while evaluating return of f1()/f2(), in C f2()…
2108e54 
… can also be evaluated first leading to reciprocal of actual desired result
@zuhu2195 zuhu2195 changed the title bug fix for evaluation of random_f and random_standard_cauchy in distributions.c BUG: fix for evaluation of random_f and random_standard_cauchy in distributions.c Aug 20, 2025
@mattip
Copy link
Member

mattip commented Aug 20, 2025

Makes sense, as the standard is clear that order of evaluation is arbitrary. In practice, it would seem to be consistent, but this PR corrects a possible defect.

WarrenWeckesser
WarrenWeckesser approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

To be clear: the potentially different compiler-dependent behaviors do not mean that the stream of random variates will be incorrect. Whether the numerator or denominator is evaluated first does not affect the correctness of the calculation. The behavior being compiler-dependent means that the streams of variates generated by code compiled with different compilers might be different. But the different streams are both "correct" (in as much as both are numerical approximations of the underlying random process).

Having said that, I agree that it is a good idea to fix the ambiguous order of evaluation. While in general we can't guarantee identical streams across different platforms and compilers, it seems reasonable to remove potential sources of difference when we can.

We already have legacy implementations for the f and standard_cauchy calculations, so this change only affects the Generator methods.

@melissawm melissawm moved this to Awaiting a code review in NumPy first-time contributor PRs Aug 20, 2025
@zuhu2195
Copy link
Author

zuhu2195 commented Aug 20, 2025 •
edited
Loading

The correctness of calculation can be impacted if there is an argument whose state changes from one call to another within the same function. Like bitgen_state in random_f(). I suggest we fix legacy_f and legacy_standard_cauchy as well.

@zuhu2195
Copy link
Author

I have added another commit for legacy_f and legacy_standard_cauchy, this keeps both the functions in sync and consistent.

@WarrenWeckesser
Copy link
Member

WarrenWeckesser commented Aug 20, 2025 •
edited
Loading

The correctness of calculation can be impacted if there is an argument whose state changes from one call to another within the same function. Like bitgen_state in random_f().

The result is certainly affected, but in this case either order of evaluation generates a correct random variate.

I suggest we fix legacy_f and legacy_standard_cauchy as well.

Generally we don't touch the legacy code; it is considered "frozen". Please revert your changes to legacy_f and legacy_standard_cauchy. (Sorry, I should have mentioned that in my previous comment about the legacy code.)

@zuhu2195
Copy link
Author

zuhu2195 commented Aug 20, 2025 •
edited
Loading

ok, will revert it. My platform is NonStop OS and these functions(legacy_f and legacy_standard_cauchy) also seem to affect the output of np.random.f in my case(giving non standard output). Will go with a local fix for legacy functions.

@mattip
Copy link
Member

mattip commented Aug 20, 2025

We care that the output of random_f and random_standard_cauchy represent the underlying random distributions. We make efforts to also make the results reproducible across different platforms and compilers, but that is not guaranteed and non-reproducibility does not make the results incorrect.

jorenham
jorenham reviewed Aug 20, 2025
View reviewed changes
zuhu2195 and others added 2 commits August 20, 2025 22:27
@zuhu2195 @jorenham
Update numpy/random/src/distributions/distributions.c
12246bf 
committing suggestion for random_f

Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
@zuhu2195 @jorenham
Update numpy/random/src/distributions/distributions.c
0ff3579 
committing suggestion for random_standard_cauchy

Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorenham jorenham jorenham left review comments

@WarrenWeckesser WarrenWeckesser WarrenWeckesser approved these changes

Assignees
No one assigned
Labels
component: numpy.random
Projects
NumPy first-time contributor PRs
Status: Awaiting a code review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zuhu2195 @mattip @WarrenWeckesser @jorenham