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

Feature/2224 stan includes #2289

Merged
merged 38 commits into from Jun 15, 2017

Conversation

Projects
None yet
4 participants
@bob-carpenter
Contributor

bob-carpenter commented Apr 24, 2017

Submisison Checklist

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

Summary

Add include mechanism to Stan language.

Intended Effect

Allow includes with #include <file-path> very much like the ones in C++.

Saves all the paths to report original file and line number and include history with error reports in both the parser and at run time.

Checks for circularity to avoid blowing out memory.

How to Verify

Unit tests.

Side Effects

Upstream interfaces will need to specify a sequence of paths in which to find include files.

Documentation

Yes. Manual and code.

Reviewer Suggestions

Anyone.

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):

Columbia University

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

bob-carpenter added some commits Feb 11, 2017

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik May 4, 2017

Member

Jenkins, retest this please.

Member

syclik commented May 4, 2017

Jenkins, retest this please.

@syclik

Looks great! A couple minor changes.

Show outdated Hide outdated src/docs/stan-reference/language.tex Outdated
Show outdated Hide outdated src/stan/io/program_reader.hpp Outdated
Show outdated Hide outdated src/stan/io/program_reader.hpp Outdated
@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik May 4, 2017

Member

@bob-carpenter, is there an easy way to generate a separate file with the include files?

It's ok if it requires a call to a second function.

From CmdStan, unless we know what the main .stan program calls, we won't know to rebuild the executable on a change to any of the includes without having a list.

Member

syclik commented May 4, 2017

@bob-carpenter, is there an easy way to generate a separate file with the include files?

It's ok if it requires a call to a second function.

From CmdStan, unless we know what the main .stan program calls, we won't know to rebuild the executable on a change to any of the includes without having a list.

@wds15

This comment has been minimized.

Show comment
Hide comment
@wds15

wds15 May 5, 2017

Should we consider to built some of the rstanarm models using this functionality as a test?

I think rstanarm stuff does make heavy use of includes and would be a good benchmark.

wds15 commented May 5, 2017

Should we consider to built some of the rstanarm models using this functionality as a test?

I think rstanarm stuff does make heavy use of includes and would be a good benchmark.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter May 5, 2017

Contributor
Contributor

bob-carpenter commented May 5, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jun 11, 2017

Contributor

Ack. I hadn't realized this never got merged and now it has comments. I can address the main comments from @syclik. If I understand what @syclik is asking for here

#2289 (comment)

it would be a list of files like this:

main.stan
foo/bar/baz.stan
...

with one relative path per file (the relative paths of the includes will be based on however they're found)? I can do that. It will require us to take another argument to the compiler and parser in the form of a std::ostream& and the files will be written one per line in ASCII with forward-slashes as delimeters (or whatever might have been input if it's mixed on Windows) terminated by \n with a final \n.

@syclik, could you confirm that's what you wanted?

Contributor

bob-carpenter commented Jun 11, 2017

Ack. I hadn't realized this never got merged and now it has comments. I can address the main comments from @syclik. If I understand what @syclik is asking for here

#2289 (comment)

it would be a list of files like this:

main.stan
foo/bar/baz.stan
...

with one relative path per file (the relative paths of the includes will be based on however they're found)? I can do that. It will require us to take another argument to the compiler and parser in the form of a std::ostream& and the files will be written one per line in ASCII with forward-slashes as delimeters (or whatever might have been input if it's mixed on Windows) terminated by \n with a final \n.

@syclik, could you confirm that's what you wanted?

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Jun 12, 2017

Member

@syclik, could you confirm that's what you wanted?

@bob-carpenter, yes. If we had that, we could get the makefile in CmdStan to only build the executable when something in the list of files has changed. If not, we'd have to rebuild the executable every time just to be safe or have the user keep track on their own.

We don't need to do it for this pull request if it takes a bit of effort. I can create a new feature request.

Member

syclik commented Jun 12, 2017

@syclik, could you confirm that's what you wanted?

@bob-carpenter, yes. If we had that, we could get the makefile in CmdStan to only build the executable when something in the list of files has changed. If not, we'd have to rebuild the executable every time just to be safe or have the user keep track on their own.

We don't need to do it for this pull request if it takes a bit of effort. I can create a new feature request.

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jun 13, 2017

Member

made minor changes requested per code review.
I could take on suggestion: #2289 (comment), but would prefer to do that in its own issue.

Member

mitzimorris commented Jun 13, 2017

made minor changes requested per code review.
I could take on suggestion: #2289 (comment), but would prefer to do that in its own issue.

@mitzimorris

This comment has been minimized.

Show comment
Hide comment
@mitzimorris

mitzimorris Jun 14, 2017

Member

filed new issue describing list of included files for make #2324

Member

mitzimorris commented Jun 14, 2017

filed new issue describing list of included files for make #2324

@syclik

syclik approved these changes Jun 14, 2017

@mitzimorris mitzimorris merged commit 9470fc1 into develop Jun 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@mitzimorris mitzimorris deleted the feature/2224-stan-includes branch Jun 15, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jun 15, 2017

Contributor

Thanks!

Contributor

bob-carpenter commented Jun 15, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment