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

[WIP] Add expression testing framework #2933

Closed
wants to merge 10 commits into from

Conversation

t4c1
Copy link
Collaborator

@t4c1 t4c1 commented Jun 29, 2020

Submission Checklist

  • Run unit tests: ./runTests.py src/test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary

Addes a testing framework that checks that functions from stan math that are exposed to stan language work when given Eigen expressions as inputs. The tests are generated from the list of signatures stanc3 supports. For now we have a list of signatures that are skipped. As more functions support working with expressions we will remove them from the llist and eventually remove the list itself.

For every signature prim, rev and fwd instantinations are tested (with all scalars of type double/var/fvar<double>). Tests check the following:

  • signatures can be compiled with expressions arguments
  • results when given expressions are same as when given plain matrices (including derivatives)
  • functions evaluate expressions at most once

TODO:

  • replace list of signatures with a call to stanc3 (once this repo uses stanc3)
  • add support for generating tests to makefile
  • make tests run on Jenkins

Intended Effect

How to Verify

Side Effects

If a new function is exposed to stan language it will need to work with eigen expressions. Specifically every argument that can be of Eigen type (matrix, vector and row_vector in stan language) need to also be able to accept expressions. That is unless we add it to the list of exceptions, which should not be allowed - the list should only be getting shorter.

Documentation

None for now. I'll add doc where requested by reviewer.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Tadej Ciglarič

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@rok-cesnovar
Copy link
Member

  • replace list of signatures with a call to stanc3 (once this repo uses stanc3)

This is in motion. Currently waiting for #2761 which just needs a review. Then I will open a PR that will download stanc3 instead of compiling stanc2 and use that to compile models. Then you can proceed here as far as that is concerned. There will be other PRs that will reorganize some of the testing but that does not conflict with these efforts.

@rok-cesnovar
Copy link
Member

@serban-nicusor-toptal Why are all Jenkins changes on this PR ignored? Is it because @t4c1 does not have write rights to this repo? Thanks.

@serban-nicusor-toptal
Copy link
Contributor

You are right @rok-cesnovar see this thread.

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Jul 14, 2020

Is it possible to give @t4c1 the same permissions he has on Math?

EDIT:
Also thanks!

@serban-nicusor-toptal
Copy link
Contributor

You're welcome!
Added to the stan team, @t4c1 you should receive an email shortly.

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Jul 14, 2020

Still unable to change Jenkinsfile here on this branch. Any ideas Nic?

EDIT: I can change it but the changes are not respected. But I know I could change it on your branch last time.

@serban-nicusor-toptal
Copy link
Contributor

Can you please push a small change now to test ? Thanks

This reverts commit 672ed98.
@serban-nicusor-toptal
Copy link
Contributor

Jenkins still sees it as untrusted.

Loading trusted files from base branch develop at ef2e811498593ab173b807dddaa15cb15ea82d77 rather than 9bd4a6212364d0470c1560f08526193f45669263
Obtained Jenkinsfile from ef2e811498593ab173b807dddaa15cb15ea82d77
‘Jenkinsfile’ has been modified in an untrusted revision

Give me a moment to understand why, if all fails we can try to re-do the PR in a minute,

@rok-cesnovar
Copy link
Member

Is it maybe because the change is coming from a fork?

@serban-nicusor-toptal
Copy link
Contributor

It should only require write permissions of the author for stan, shouldn't for the fork source.

@t4c1 t4c1 closed this Jul 14, 2020
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.

3 participants