Map_rect concurrent about to land

[wds15]

@andre.pfeuffer

I suppose I can use threading without installing MPI. Is this true?

Yes, that’s correct.

@andre.pfeuffer

And what happen, if both threading and MPI being enabled at the same time?

Technically possible, but I have seen segfaults in this configuration which may be possible to avoid, but I haven’t had the time to dig into this. I would not use this at the moment. I think the problems are related to MPI and threading implementations not harmonizing such that it should be possible to get it to work with the right MPI and compiler setup.

@andre.pfeuffer

Are they conflicting?

Not really. In any case, the MPI will take precedence over threading.

@andre.pfeuffer

Then recompile and map_rect will work in parallel?

During compilation it only matters that you turn on the compiler flags which you list. The STAN_NUM_THREADS is only relevant during runtime (so when you actually start your stan program)

You can put the STAN_NUM_THREADS into you .bashrc or set it when you start your stan program. So this also works on Linux with the Stan program foo:

# take what has been globally defined:
./foo ...
# define STAN_NUM_THREADS for this run
STAN_NUM_THREADS=16 ./foo

I hope this make it clear how to use it. Let me know if not.

What type of model are you using this on? I am just curious.

Best,
Sebastian

[andre.pfeuffer]

All clear now.

@wds15

What type of model are you using this on? I am just curious.

Sure, a Gaussian Process based spatial temporal model on the bivariate Sichel Poisson distribution
by Gillian Z. Stein & June M. Juritz

  real besselK(real z, real v) {
   if(v==-0.5||v==0.5)
    return (sqrt(pi()/(2. *z))*exp(-z));
    return (besselK(z, v - 2) + 2. *(v-1)/z * besselK(z, v - 1));
 
  }
  real bipoissonsichel_log(int x1, int x2, real a, real zeta1, real zeta2, real g) {
    real w = sqrt(a*a + 1) - 1;
    return  log(besselK(sqrt(w*(w+2*zeta1+2*zeta2)), x1 + x2 + g)) -
            log(besselK(w, g)) + 
            ((x1 + x2 + g)*0.5) * log(w / (w + 2*zeta1 + 2*zeta2))
          + x1 * log(zeta1) + x2 * log(zeta2) - lgamma(x1+1) - lgamma(x2+1);
  
  }
  //real Rv(real x, real g) {
  //  return besselK(x,g+1) / besselK(x, g);
  //}

[martinmodrak]

Sorry for jumping in waaaay late, just a minor suggestion. If it is too late to change this, I totally understand and it’s not a big thing.

@wds15

Not setting STAN_NUM_THREADS or setting it to a non-numeric value results in a single thread being used

I generally rather libraries fail on unusual stuff instead of trying to remedy in some way - e.g. if I make a typo and write 1O instead of 10 (or R instead of 4 which I actually write from time to time) I would expect Stan to either fail or at least show a warning. Or is there some other rationale for this?

[wds15]

@martinmodrak

I generally rather libraries fail on unusual stuff instead of trying to remedy in some way - e.g. if I make a typo and write 1O instead of 10 (or R instead of 4 which I actually write from time to time) I would expect Stan to either fail or at least show a warning. Or is there some other rationale for this?

Uhm… this is too late for now. It’s released already and everyone was fine with the behavior proposed. Since the hasn’t been used too much used yet, we could discuss if we change the behavior for the case of wrong values (in any case I would still stick to the non-defining behavior as suggested).

[martinmodrak]

@wds15

this is too late for now

I thought this is the case.

And yes I am totally okay with “STAN_NUM_THREADS not defined → single thread”, I only question the behavior for non-numeric values. A warning might be totally backwards compatible?

And really it’s not a big deal, just my small philosophical itch. Thanks for working on this project!

[syclik]

@Bob_Carpenter, can you coordinate with @wds15 to get the necessary documentation in?

@wds15: can you clarify what behavior you’re taking about? I agree that we should fail with the best error message possible instead of going forward. I didn’t know what behavior was proposed.

[wds15]

@martinmodrak

A warning might be totally backwards compatible?

Yes… but issuing a message from that code location would probably be more effort than just changing the behavior. I can imagine that others would agree to your thinking - it does make sense.

So whenever we cannot convert STAN_NUM_THREADS to an integer (or that integer is less than -1), then we can fire an exception which terminates the program. Fine by me. Would you mind filing an issue for that?

I can tell you that MPI was quite a project to get through. I am glad it’s out now and we are now in “cleaning” mode.

[wds15]

@syclik

@Bob_Carpenter, can you coordinate with @wds15 to get the necessary documentation in?

It is documented in our doxygen doc. More?

@syclik

I agree that we should fail with the best error message possible instead of going forward. I didn’t know what behavior was proposed.

I discussed this in a stan meeting and the PR was reviewed.

Like I say, we can probably change this behavior via an issue+PR?

[Bob_Carpenter]

@martinmodrak

I generally rather libraries fail on unusual stuff instead of trying to remedy in some way

I completely agree and we should change this behavior. Specification of anything other than a non-negative integer should throw an exception and stop the process with an informative warning message.

@wds15

everyone was fine with the behavior proposed

@syclik

I agree that we should fail with the best error message possible instead of going forward. I didn’t know what behavior was proposed.

I also didn’t catch that detail.

I very strongly believe we shoudl flag illegal arguments, stop processes, and report error messages.

@syclik

@Bob_Carpenter, can you coordinate with @wds15 to get the necessary documentation in?

Sure. @wds15—after you update the Wiki, ping me and I can test it all from the top on my local setup.

[Bob_Carpenter]

@wds15

we can probably change this behavior via an issue+PR?

I think that’s what we should do. It’s easy to miss these little details in the face of these big PRs—I don’t think there’s anything we can do to avoid it—it’s impossible to put a big red box around every aspect of a PR.

[syclik]

@wds15

I discussed this in a stan meeting and the PR was reviewed.

I understand, but I don’t know if the behavior was clear to all that was reviewing it. It’d just help, in the future, to summarize decisions like that.

Btw, there’s no blame or anything here. It’s just a detail that was missed. Especially to anyone that wasn’t at the meeting in particular.

@Bob_Carpenter

I completely agree and we should change this behavior. Specification of anything other than a non-negative integer should throw an exception and stop the process with an informative warning message.

I know, bad etiquette, but +1.

@Bob_Carpenter

I think that’s what we should do. It’s easy to miss these little details in the face of these big PRs—I don’t think there’s anything we can do to avoid it—it’s impossible to put a big red box around every aspect of a PR.

Agreed. Sometimes the downstream consequence of these little behaviors aren’t known until we run into them.

@wds15

I am glad it’s out now and we are now in “cleaning” mode.

@wds15, a request, please. Please don’t think of things getting into Math and Stan as prototypes where it requires cleaning afterwards. That really puts a lot of burden on the rest of the team in technical debt. A lot more than you might imagine. If you need help with design or implementation, please ask for it. We just can’t be pushing through code that’s not quite right for the core components.

That said, thanks for putting it together! It wouldn’t have happened without your push.

---

@wds15 and @Bob_Carpenter – thanks for finishing this out.

[martinmodrak]

Filed issue: https://github.com/stan-dev/stan/issues/2591

@wds15: sorry for giving you more work and thanks for your effort. I can also try to handle this if you don’t have the capacity (I don’t know the codebase, but should be minor change)

[wds15]

@syclik

Please don’t think of things getting into Math and Stan as prototypes where it requires cleaning afterwards. That really puts a lot of burden on the rest of the team in technical debt.

My wording wasn’t ideal there. I would not put something immature into stan-math… what maybe describes it better is that by now that I have gone through implementing this thing (and also having worked on other codes) I would probably be able to come up with a more refined design of the code. I do think that map_rect is well implemented; I certainly tried to achieve that.

@martinmodrak

I can also try to handle this if you don’t have the capacity (I don’t know the codebase, but should be minor change)

If you could do that, then that would be appreciated. This should be an easy one to fix as it is about to pinpoint the location and throw an exception when appropriate (Ok, and lookup the doc of the atoi function).

[Bob_Carpenter]

@wds15

My wording wasn’t ideal there. I would not put something immature into stan-math… what maybe describes it better is that by now that I have gone through implementing this thing

I understood that’s what you meant. And thanks for bearing with this.

I don’t think anyone expected MPI to launch with zero hiccups.

@wds15

lookup the doc of the atoi function

Nope, that won’t work, see http://www.cplusplus.com/reference/cstdlib/atoi/

If str does not point to a valid C-string, or if the converted value would be out of the range of values representable by an int, it causes undefined behavior.

I would suggest boost::lexical_cast—it throws a bad_lexical_cast when the conversion fails which you can catch or propagate.

[syclik]

Thanks again, @wds15! Sorry if I was being too literal.