Skip to content

Ignore lchoose in expression tests#2607

Merged
rok-cesnovar merged 1 commit intodevelopfrom
add_lchoose_exception
Oct 25, 2021
Merged

Ignore lchoose in expression tests#2607
rok-cesnovar merged 1 commit intodevelopfrom
add_lchoose_exception

Conversation

@rok-cesnovar
Copy link
Copy Markdown
Member

Summary

stan-dev/stanc3#1010 will add lchoose for vectors, row vectors and arrays. Because lchoose is generated as binomial_coefficient_log in stanc3 we need to add an exception to the expressions tests.

Tests

/

Side Effects

/

Checklist

  • Copyright holder: Rok Češnovar

Copy link
Copy Markdown
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

LGTM

Why did this pass tests before? Lchoose was still around before the change

@rok-cesnovar
Copy link
Copy Markdown
Member Author

rok-cesnovar commented Oct 25, 2021

Because expression templating is used with Eigen inputs (vectors, row vectors, matrices) and signatures without those inputs do not require testing expressions (previously it was only scalars I believe?)

@WardBrian
Copy link
Copy Markdown
Member

That makes sense. I wonder if this is why the signature was left un vectorized, the tests failed and they just changed it back to scalars only

@rok-cesnovar
Copy link
Copy Markdown
Member Author

rok-cesnovar commented Oct 25, 2021

I doubt it, expressions have been live for 2 release cycles so the expressions testing was added then as well and I dont remember seeing a lchoose PR and I have been following them quite closely in the last year or so. Always an option I missed stuff though :)

@WardBrian
Copy link
Copy Markdown
Member

Running the full tests on this seems rather unnecessary, doesn't it?

@rok-cesnovar
Copy link
Copy Markdown
Member Author

Yeah, probably true, though sig_utils is used in expressions tests in Math as well so technically its good if we run it. We could force it to just run expressions tests if the only change is in this file, but not sure its worth the hassle for an edge case.

The bigger issue is #2324 (comment)

Given that expressions tests passed we can go ahead and merge it though.

@rok-cesnovar rok-cesnovar merged commit 20224ff into develop Oct 25, 2021
@rok-cesnovar rok-cesnovar deleted the add_lchoose_exception branch October 25, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants