Better error messages for incomplete probability calls

[adamhaber]

Fixes #987 .

Not sure what would be the best way to test this. I've built and ran stanc on

data { 
}
model {  
    // known family, known suffix, not implemented  
    target += von_mises_cdf(1|0,1);
    target += von_mises_lcdf(1|0,1);
    target += von_mises_lccdf(1|0,1);

    target += dirichlet_cdf([0.2, 0.1, 0.7]|[1,1,1]);
    target += dirichlet_lcdf([0.2, 0.1, 0.7]|[1,1,1]);
    target += dirichlet_lccdf([0.2, 0.1, 0.7]|[1,1,1]);
}

and I'm getting the right error message but only on the first line.

Submission Checklist

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here:
  • OR, no user-facing changes were made

Release notes

Improved error messages for incomplete probability library calls.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[rok-cesnovar]

and I'm getting the right error message but only on the first line.

Isn't that what happens also if you use 3 non-existent functions? Won't it only error for the first one?

[adamhaber]

Isn't that what happens also if you use 3 non-existent functions? Won't it only error for the first one?

Definitely! Just wanted to ask what would be a good way to test, in light of this (since I can't throw all the different cases in one large file) :-)

[rok-cesnovar]

I guess a few short test files, each with a faulty signature in a subfolder should be fine.

[rok-cesnovar]

A question on which I don't have an answer to, is if we envision a case where one of the non-lpdf/lpmf suffixed functions is implemented, but we don't have an lpdf/lpmf function implementation. So, things like there is a foo_rng but no foo_lpdf.

We don't right now and that would probably be bad anyway, so I don't think we need to handle it. Just thinking out loud.

[rok-cesnovar]

Outside of the error message, for which I would like someone else's input, this all looks good.

[nhuurre]

a case where one of the non-lpdf/lpmf suffixed functions is implemented, but we don't have an lpdf/lpmf function implementation.

That would be very bad, I agree. It would also impact twiddle statements.

I think it's worth special-casing _lpdf vs _lpmf so that if e.g. someone writes binomial_lpdf then the error says you must use binomial_lpmf instead.

[rok-cesnovar]

I think it's worth special-casing _lpdf vs _lpmf so that if e.g. someone writes binomial_lpdf then the error says you must use binomial_lpmf instead.

Oh nice, yeah. That would also be great, also include lupdf vs lupmf if we add that.

[WardBrian]

Our of curiosity, what happens if there’s a user defined function bar_lpdf and a user tries to call bar_rng?

Also, I think the original issue suggested listing the suffixes which are available, do we still want that?

[nhuurre]

Oh and one more thing before I forget: Stan_math_signatures.distributions list does not contain all the Stan math distributions.
Take for example categorical.
Ok, categorical_lpmf is from distributions

stanc3/src/middle/Stan_math_signatures.ml

Lines 222 to 223 in b0f2bd3

	; ([Lpmf], "categorical", [DVInt; DVector], AoS)
	; ([Lpmf], "categorical_logit", [DVInt; DVector], AoS)

However, note that the above suggest categorical_rng does not exist but it is actually added later

stanc3/src/middle/Stan_math_signatures.ml

Lines 977 to 983 in b0f2bd3

	add_unqualified ("categorical_rng", ReturnType UInt, [UVector], AoS) ;
	add_unqualified ("categorical_logit_rng", ReturnType UInt, [UVector], AoS) ;
	add_unqualified
	( "categorical_logit_glm_lpmf"
	, ReturnType UReal
	, [UArray UInt; UMatrix; UVector; UMatrix]
	, AoS ) ;

where we also have categorical_logit_glm which is completely absent in distributions.

Many more distribution functions are scattered around the file. These should all be consolidated in the distributions list if at all possible.

[adamhaber]

Our of curiosity, what happens if there’s a user defined function bar_lpdf and a user tries to call bar_rng?

For:

functions {
    real bar_lpdf(real x) {
        return(x);
    }
}
model {  
    target += bar_lcdf(1|0,1);
}

I get:

Semantic error in './tmp.stan', line 7, column 14 to column 29:
   -------------------------------------------------
     5:  }
     6:  model {  
     7:      target += bar_lcdf(1|0,1);
                       ^
     8:  }
   -------------------------------------------------

A returning function was expected but an undeclared identifier 'bar_lcdf' was supplied.

Also, I think the original issue suggested listing the suffixes which are available, do we still want that?

Another option would be to add a link to the functions-reference docs (I personally like compiler errors that do that), but I'd be happy to add whatever people think is right.

[WardBrian]

This will be very nice for users I think. A few comments, and there are a few tests I'd like added:

user_defined:

functions {
   real bar_lpdf(real x){
     return 1.0;
   }
}

model {
   target += bar_lpmf(19.2);
}

user_defined_no_rng

functions {
   real bar_lpdf(real x){
     return 1.0;
   }
}

generated quantities {
   real x = bar_rng(10);
}

(also, if there are any builtin distributions without RNG's we should test one. Not sure)

user_defined_no_cdf:

functions {
   real bar_lpdf(real x){
     return 1.0;
   }
}

model{
    target += bar_lcdf(1|0,1);
}

and something that tests the older/deprecated _cdf_log/_ccdf_log, like

model {
    target += von_mises_ccdf_log(1, 0,1);
}

These currently output a variety of things, and we should discuss what we want them to say

[WardBrian]

Here's a test case for a distribution w/o rng suffix:

generated quantities {
   real x = multi_gp_cholesky_rng([[0,0],[0,0]], [1,0]);
}

[WardBrian]

Partially because of @nhuurre's comment above and partially because of the output for user-defined functions, I wonder if we just shouldn't test if the function is a 'known' distribution family if it has a known suffix? It would mean userdefined functions would also get the output, but maybe that's good?

[nhuurre]

if we just shouldn't test if the function is a 'known' distribution family if it has a known suffix?

From the original issue

can become frustrating assuming that there’s a spelling error or other typo somewhere and not just a non-implemented functionality.

It would be confusing the other way too, if a legit typo gets reported as unimplemented functionality.

[WardBrian]

Good point. I do wonder if the user-defined family should be handled though, like if someone defines bar_lpdf should we give this error when they try to use bar_lcdf?

[adamhaber]

@WardBrian added the tests you've requested, I think the behaviour is expected but would appreciate an extra look at this.

[WardBrian]

#995 is in, which hopefully lets you handle the suffixes a bit nicer. Looking at the tests now

[WardBrian]

Looks much cleaner now! I think there is one remaining issue we should fix before merging

[WardBrian]

Thanks @adamhaber !