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

Feature optional profiling include paths #54

Merged
merged 12 commits into from May 19, 2022

Conversation

adrian-lison
Copy link
Collaborator

This PR extends the optional profiling also to .stan files in the include paths, and moves the whole functionality to a separate function which is called by enw_model if required. All manipulated .stan files with profiling statements removed are stored in the same temporary directory.

Implementation details: By using the same folder structure in the temporary directory as in the original include paths, the relative include paths still apply and the model code is not further touched aside from removing the profiling. This should make the approach very robust. Also, the names of the main model and the included file paths are now directly mirrored, so that warnings or errors from stanc stating a filepath allow the user to pinpoint the corresponding original .stan file.

This PR also adds more detailed profiling statements to the likelihood parts of the model.

@adrian-lison adrian-lison added the enhancement New feature or request label May 10, 2022
@adrian-lison adrian-lison self-assigned this May 10, 2022
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #54 (b71ac73) into main (d49604e) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff          @@
##            main     #54   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         13      13           
  Lines        655     685   +30     
=====================================
- Misses       655     685   +30     
Impacted Files Coverage Δ
R/model-tools.R 0.00% <0.00%> (ø)
R/model.R 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d49604e...b71ac73. Read the comment docs.

@adrian-lison adrian-lison requested a review from seabbs May 10, 2022 10:17
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looks really nice and as I send before worth considering for an upstream contribution to cmsdstanr. I have some very boring style points and would like the linting sorted out 😄

R/model-tools.R Outdated Show resolved Hide resolved
R/model-tools.R Outdated Show resolved Hide resolved
R/model-tools.R Outdated Show resolved Hide resolved
inst/stan/functions/obs_lpmf.stan Show resolved Hide resolved
@adrian-lison
Copy link
Collaborator Author

Thanks for reviewing @seabbs :) I have improved the formatting as suggested. Will consider proposing it to cmdstanr also :)

@adrian-lison adrian-lison requested a review from seabbs May 18, 2022 07:52
@seabbs
Copy link
Collaborator

seabbs commented May 19, 2022

/document

seabbs
seabbs previously approved these changes May 19, 2022
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM to me. Just updating the docs and then will merge (and add news etc direct to master).

@seabbs seabbs merged commit e996d0d into main May 19, 2022
@seabbs seabbs deleted the feature-optional-profiling-include-paths branch May 19, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants