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

Randpoisson coding error; uses while loop not do-while #235

Open
cmarooney-stfc opened this issue Sep 23, 2020 · 2 comments
Open

Randpoisson coding error; uses while loop not do-while #235

cmarooney-stfc opened this issue Sep 23, 2020 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@cmarooney-stfc
Copy link
Collaborator

cmarooney-stfc commented Sep 23, 2020

In herbert_core\utilities\math\randpoisson.m, the while loop in lines 25-28 should be a do-while loop. For arg lam==0, this gives a return value of -1. With the correction to do-while, it gives 0, which agrees with the native MATLAB poissrnd. Using the correct form from the Numerical Recipes versions for F90 and C, also get 0 (but not F77 which is unusable as it contains a goto :-)).

Alternative fixes are:

  • code the do-while loop
  • use native MATLAB poissrnd
    The latter would be easier as then we do not need to test the result (of course MATLAB is always right!) whereas for randpoisson we should add tests to ensure it is correct.
@cmarooney-stfc cmarooney-stfc self-assigned this Sep 23, 2020
@nickbattam-tessella nickbattam-tessella added the bug Something isn't working label Sep 23, 2020
@nickbattam-tessella
Copy link
Member

Toby added this implementation for specific performance reasons managing issues with the then-available alternatives.

If the function is replaced suitable performance metrics should be generated to prove the native MATLAB implementation (which presumably didn't exist when this function was created in 2006) is at least as performant.

@tgperring
Copy link
Collaborator

The problem is that poissrnd is a function in the statistics and machine learning toolbox. We should only be using matlab functions that are in the main Matlab, and not in any of the toolboxes because we cannot count on users having particular toolboxes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants