-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Profiling #434
Profiling #434
Conversation
This is not just ready yet, missing docs and test but need feedback on the API. @bbbales2 if you have a few moments, mind looking at this? fit$profiles() returns a list of data frames |
@rok-cesnovar works for me. There probably should be a |
Currently the situation is:
So
|
Thanks. I agree. @jgabry are you good with these names fit$profiles()
fit$profile_files()
fit$save_profile_files() Just double checking with you. Ben agreed to review the functionalities. |
Update actually
|
@jgabry this all works great for me. It would be good to get this reviewed and merged ASAP so we can test before release (next Monday, the 25th) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few comments/questions
Actually don't merge this yet. I think there's a problem:
|
I am getting a weird error with vignettes that I didnt touch. Checking. |
Yeah I think it's the same error as the one I got running cmdstanr_example() |
I get a lot of test failures too if I run the unit tests locally |
Can you try again? |
Nice, that seems to fix the error I was seeing. |
Once this is in I am going to try my luck making a vignette. What is the easiest way to build the HTML and everything we need to publish it here? |
Cool. Do you mean how do we include a new vignette on the CmdStanR website? |
Yeah. Never made a vignette before.. |
Not complicated but a few things required. How about I demonstrate by creating a branch for the new vignette and doing everything necessary except writing the content? Then you can see what I did so you can do it yourself next time you want to add a vignette. Does that work? |
That works perfectly if you have the time. Thank you! |
Yeah I have a few min now so I can do that |
Done: #435 |
I think this is failing now because my previous suggestion was only half right! We need to check:
So something like this: profile_files = function(include_failed = FALSE) {
files <- private$profile_files_
if (!length(files) || !any(file.exists(files))) {
stop(
"No profile files found. ",
"The model that produced the fit did not use any profiling.",
call. = FALSE
)
}
if (include_failed) {
files
} else {
ok <- self$procs$is_finished() | self$procs$is_queued()
files[ok]
}
} |
Let's see what happens if I push that |
Now I think the test coverage is only failing because it's not using the release candidate. I guess there are two options: we could change the installation to the RC like you did for the R CMD check workflow, or we could only run the failing tests if version >= 2.26. Preference? |
This is probably better. |
Codecov Report
@@ Coverage Diff @@
## master #434 +/- ##
==========================================
- Coverage 93.12% 89.93% -3.20%
==========================================
Files 12 12
Lines 2822 2870 +48
==========================================
- Hits 2628 2581 -47
- Misses 194 289 +95
Continue to review full report at Codecov.
|
Thank you so much for profiling functionality! I hope to give it a test drive soon. |
Summary
Adds support for profiling in cmdstanr.
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):
Rok Češnovar
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: