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

Adapt to new posterior database structure #14

Merged
merged 10 commits into from
Aug 10, 2019
Merged

Conversation

eerolinna
Copy link
Contributor

@eerolinna eerolinna commented Aug 8, 2019

This pull request changes to new posterior database structure and adapts rpackage to the new structure.

Fixes #8

@eerolinna
Copy link
Contributor Author

The problem with this solution is that we no longer have access to the data names and model names from po. One way to fix this would be to add

po$data_name <- po$data
po$model_name <- po$data

to posterior in posterior.R

This is maybe not the best solution still though

@eerolinna
Copy link
Contributor Author

Another way would be not to overwrite po$model and po$data but instead make new fields like po$model_info and po$data_info

A third way would be to not attach the info file contents to po but load them on-demand. Then for example in model_code_file_path

https://github.com/MansMeg/posterior_database/blob/96dcb71330f376edecf9b7f8613c067d0d58cb85/rpackage/R/model_code.R#L12

would become something like

model_info <- load_model_info(x$model)
mcfp <- file.path(x$pdb$path,  model_info[[framework]]) 

@eerolinna
Copy link
Contributor Author

Now this PR doesn't overwrite anything.

The slots in the posterior object are now

  • model_name: from posterior json file
  • data_name: from posterior json file
  • model_info: contents of model info file
  • data_info: contents of data info file

@eerolinna eerolinna changed the title Adapt rpackage to new posterior database structure Adapt to new posterior database structure Aug 9, 2019
@eerolinna
Copy link
Contributor Author

This should include everything that is needed now. I think you haven't added the contributor etc slots yet so I couldn't include them here. Would be great if you can first merge this and then add the new slots

json file inside them had name with lowercase n instead of capital N, which meant that R package couldn't find them
@eerolinna
Copy link
Contributor Author

I found a problem in master branch that was causing tests to fail, some dataset zip files had json files inside them with wrong filename (lowercase n instead of capital N) that caused stan_data(po) to not find the data file, I corrected that here also.

@MansMeg MansMeg merged commit 3829c1b into stan-dev:master Aug 10, 2019
@MansMeg
Copy link
Collaborator

MansMeg commented Aug 10, 2019

Great! I had problem finding that problem yesterday.

@eerolinna eerolinna deleted the patch-2 branch August 21, 2019 10:30
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.

Adding models of different frameworks needs duplication in many posterior files
2 participants