Skip to content
Snippets Groups Projects

Merge in error-checking from lalsuite version

Closed Merge in error-checking from lalsuite version
john-veitch/ligo.skymap:merge_lalinference_changes into master
3 unresolved threads
Closed John Douglas Veitch requested to merge john-veitch/ligo.skymap:merge_lalinference_changes into master
3 unresolved threads

This merges in the changes requested by @leo-singer in lalsuite!474 (merged). This is mainly related to catching GSL errors when possible in the distance integrator.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
src/bayestar_sky_map.c
307 326 log_offset = 0;
308 327
309 328 params.scale = -log_offset;
310
311 {
329 size_t n = 64;
330 double abstol = 1e-8;
331 double reltol = 1e-8;
332 int relaxed=0;
333 do {
312 334 /* Maximum number of subdivisions for adaptive integration. */
313 static const size_t n = 64;
  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    Why was this moved?

  • John Douglas Veitch
    John Douglas Veitch @john-veitch ·
    Author Developer

    I had to remove the "const" keyword so that n can be increased in the section below.

  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    Would you please add to bayestar_test a case of interest for your analysis that breaks the old tolerance and subdivision limit?

    If possible, I would rather keep n and reltol as constants but increase their literal values, because this doubling of the number of subdivisions is very similar to what the qagp integrator itself does.

  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    Increasing the maximum number of iterations is not appropriate for BAYESTAR because the code is designed for low-latency operation, and must complete in a predictable amount of time.

  • Please register or sign in to reply
  • Leo P. Singer
    Leo P. Singer @leo-singer started a thread on the diff
    • src/bayestar_sky_map.c
    221 221 const double b = integrand_params->b;
    222 222 const int k = integrand_params->k;
    223 223 double ret = scale - gsl_pow_2(p / r - 0.5 * b / p);
    224 int retval=GSL_SUCCESS;
    225 gsl_sf_result result;
    226 gsl_error_handler_t *old_handler = gsl_set_error_handler_off();
    227
    224 228 if (integrand_params->cosmology)
    225 229 ret += log_dVC_dVL(r);
    230 retval = gsl_sf_exp_mult_e(
    • Leo P. Singer
      Leo P. Singer @leo-singer ·
      Maintainer

      Do these error handling changes actually have any effect? Upon an underflow error, won't the result be 0 anyway?

    • John Douglas Veitch
      John Douglas Veitch @john-veitch ·
      Author Developer

      I ran into cases where GSL was throwing it as an error and causing the code to abort.

    • John Douglas Veitch
      John Douglas Veitch @john-veitch ·
      Author Developer

      Generally speaking if the default error handler is turned off (which is recommended for production code) then you have to check the return codes anyway.

    • Leo P. Singer
      Leo P. Singer @leo-singer ·
      Maintainer

      I think that I almost always run the code with the error handler turned off. Is this not what LALInference does?

      In this case, if the error handler is turned off, checking gsl_errno is not necessary because on underflow the return value is just 0.0, as expected.

    • John Douglas Veitch
      John Douglas Veitch @john-veitch ·
      Author Developer

      Indeed, I haven't turned it off before. That would explain why I was finding a problem. If this is all un-necessary then we can scrub this merge request.

    • Leo P. Singer
      Leo P. Singer @leo-singer ·
      Maintainer

      Well, currently it is my Python wrapper that is setting the error handler. So probably the LALInference likelihood should set it.

    • Please register or sign in to reply
  • Leo P. Singer
  • Leo P. Singer
    Leo P. Singer @leo-singer started a thread on an old version of the diff
    • src/bayestar_sky_map.c
    307 326 log_offset = 0;
    308 327
    309 328 params.scale = -log_offset;
    310
    311 {
    329 size_t n = 64;
    330 double abstol = 1e-8;
  • John Douglas Veitch
  • John Douglas Veitch added 1 commit

    added 1 commit

    Compare with previous version

  • John Douglas Veitch added 1 commit

    added 1 commit

    Compare with previous version

  • John Douglas Veitch
    John Douglas Veitch @john-veitch ·
    Author Developer

    By the way, in the LALInference version I added a line to log_radial_integrator_init(),

    if(cosmology) dVC_dVL_init();

    Not sure if that is being initialised elsewhere in this version so I didn't add it, but you may find it to be a bug if not.

  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    By the way, in the LALInference version I added a line to log_radial_integrator_init(),

    if(cosmology) dVC_dVL_init();

    Not sure if that is being initialised elsewhere in this version so I didn't add it, but you may find it to be a bug if not.

    It's called by bayestar_init().

  • John Douglas Veitch added 1 commit

    added 1 commit

    • 9f909483 - Set absolute tolerance back to DBL_MIN

    Compare with previous version

  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    @john-veitch, this merge request is a year old now. Are you still interested in merging it?

  • Leo P. Singer
    Leo P. Singer @leo-singer ·
    Maintainer

    Patch seems abandoned.

  • Leo P. Singer closed

    closed

    • Please register or sign in to reply