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

Return variable names and dimensions #256

Merged
merged 8 commits into from
Jul 30, 2020
Merged

Return variable names and dimensions #256

merged 8 commits into from
Jul 30, 2020

Conversation

rok-cesnovar
Copy link
Member

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Attempt at fixing #240

For now, this just returns the variable names and dimensions as part of metadata in read_cmdstan_csv. We should add it in fit, but not sure on the name.

Example:

> f <- cmdstanr_example("logistic", method = "sample")
> csv <- read_cmdstan_csv(f$output_files())
> f1$metadata$model_params_dims
$lp__
[1] 0

$alpha
[1] 0

$beta
[1] 3

> f1$metadata$model_params_named
[1] "lp__"  "alpha" "beta"

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, Univ. of Ljubljana

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

@rok-cesnovar rok-cesnovar changed the title add dimensions and names to read_cmdstan_csv Return variable names and dimensions Jul 28, 2020
@rok-cesnovar
Copy link
Member Author

@jgabry we can hold off with this for after beta if you want. I am done for today, but if you have any feedback I can finish this tomorrow.

Mostly want ideas for names. Does metadata$model_params_dims and metadata$model_params_named work?
Would fit$draws_dims() work? Or should this be a function on draws rather than a part of fit?

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #256 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   90.78%   90.85%   +0.07%     
==========================================
  Files          11       11              
  Lines        2311     2329      +18     
==========================================
+ Hits         2098     2116      +18     
  Misses        213      213              
Impacted Files Coverage Δ
R/read_csv.R 96.28% <100.00%> (+0.04%) ⬆️
R/utils.R 70.85% <100.00%> (+2.53%) ⬆️

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 fb3703f...4dd2c18. Read the comment docs.

@jgabry
Copy link
Member

jgabry commented Jul 28, 2020

Ok cool thanks I'll take a look.

R/utils.R Outdated Show resolved Hide resolved
tests/testthat/test-utils.R Show resolved Hide resolved
R/read_csv.R Outdated Show resolved Hide resolved
@mitzimorris
Copy link
Member

Mostly want ideas for names. Does metadata$model_params_dims and metadata$model_params_named work?
Would fit$draws_dims() work? Or should this be a function on draws rather than a part of fit?

hi, could we co-ordinate w/ CmdStanPy on names? we went through this a while back - last thing I implemented there was stan-dev/cmdstanpy#252 - called everything "stan_variable".

@jgabry
Copy link
Member

jgabry commented Jul 28, 2020

Yes good point @mitzimorris!

@jgabry
Copy link
Member

jgabry commented Jul 28, 2020

@mitzimorris Does stan_var_dims() in CmdStanPy say that a variable

real alpha

has dim 0 or 1?

R/utils.R Show resolved Hide resolved
@jgabry
Copy link
Member

jgabry commented Jul 28, 2020

@rok-cesnovar I pushed a few edits and I think this is ok now other than changing the names. Are you ok with changing to "stan_variable", which is what @mitzimorris said CmdStanPy uses?

@mitzimorris
Copy link
Member

mitzimorris commented Jul 28, 2020

@mitzimorris Does stan_var_dims() in CmdStanPy say that a variable

real alpha

has dim 0 or 1?

property stan_var_dims returns a Python dict which maps
Stan program variable names to variable dimensions.
Scalar types have int value '1'. Structured types have list of dims,
e.g., program variable vector[10] foo has entry ('foo', [10]).

given fit object bernoulli_fit, stan_var_dims returns {'theta': 1}
given fit object logistic_fit, stan_var_dims returns {'beta': (2,)}

ps. thanks for asking - noticed that doc doesn't match output. damn.

@rok-cesnovar
Copy link
Member Author

@jgabry changed to stan_var_dims and stan_variables. Also added a few more tests. I think its good to go.

@jgabry
Copy link
Member

jgabry commented Jul 29, 2020

Looks good! This is good to go, just one super nitpicky thing (sorry!): I think it's slightly weird to use stan_variables but then use stan_var_dims instead of stan_variable_dims. I would prefer using either stan_vars and stan_var_dims or stan_variables and stan_variable_dims. I think I prefer the latter, but I'm fine either way, as long as it's consistent. @mitzimorris and @rok-cesnovar what do you think?

@rok-cesnovar
Copy link
Member Author

I would prefer using either stan_vars and stan_var_dims or stan_variables and stan_variable_dims.

I agree. That annoyed me a bit as well to be honest but thought that was discussed and agreed on earlier. I prefer the verbose version as well.

@jgabry jgabry mentioned this pull request Jul 29, 2020
2 tasks
@mitzimorris
Copy link
Member

agreed. will change CmdStanPy accordingly.

@rok-cesnovar
Copy link
Member Author

@jgabry renamed. Good to merge?

@jgabry
Copy link
Member

jgabry commented Jul 30, 2020

Yeah looks good. Will merge now.

@jgabry jgabry merged commit 4d058a8 into master Jul 30, 2020
@jgabry jgabry deleted the feature/model_pars branch July 30, 2020 18:36
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.

4 participants