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

Added export constructors (I used the wrong local branch name). #73

Merged
merged 5 commits into from
Mar 27, 2019

Conversation

goedman
Copy link
Collaborator

@goedman goedman commented Mar 26, 2019

Added Array and DataFrame constructors and associated tests.

These constructors are intended as being used at the back-end of mcmc workflows, thus after checking for valid samples has been completed. Both the Array and DataFrame constructors are overloaded.

Simple help is available using ?Array and ?DataFrame once MCMCChains is loaded.

Added Array and DataFrame constructors and associated tests
@trappmartin
Copy link
Member

Looks good.

However, I'm not 100% convinced why it makes sense to implement those constructors instead of simply providing convert function, e.g. convert(::Type{Array}, chn::Chains) = chn.value?
Maybe provide both?

Copy link
Member

@cpfiffer cpfiffer left a comment

Choose a reason for hiding this comment

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

I've added a couple of just minor spelling updates, but this looks excellent! Thank you!

src/constructors.jl Outdated Show resolved Hide resolved
src/constructors.jl Outdated Show resolved Hide resolved
src/constructors.jl Outdated Show resolved Hide resolved
@cpfiffer
Copy link
Member

As to what @trappmartin said, I added a convert(::Type{Array}, chn::Chains) function just to have around. It's super basic in case someone just needs a smash-and-grab array. Rob's Array functions is much better I think for handling all the sometimes weird stuff MCMCChains has.

@goedman goedman merged commit b6fdd05 into master Mar 27, 2019
@goedman
Copy link
Collaborator Author

goedman commented Mar 27, 2019

I might have jump the gun by merging the export chains additions. My apologies, I thought I'd seen a generic approved tick.

For quite a while I've used the convert() option but often felt baffled by not having control over the exact sequence in which variables are inserted into the array or dataframe. This is particularly bothersome for arrays that don't hold the names. The other considerations are appending chains, filtering out the Missing union and above all an option to work with one or more specific sections, e.g. population wide parameters and pooled parameters.

Today I've added some short sections to the README (sampling and exports) and then we could maybe consider a new release. I just did a PR with these changes included.

I'm looking into one other component which is a DataFrame based chain summary, but that is still early on.

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.

3 participants