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

Change default dimension order to (draw, chain, params...) #40

Merged
merged 13 commits into from
Dec 11, 2022
Merged

Conversation

sethaxen
Copy link
Member

This PR changes the default dimension order from (params..., draw, chain) to (draw, chain, params...). While this is a breaking change, no user code that indexes and permutes dimensions using dimension names instead of indices will be impacted.

This changes the ordering from that adopted in #8. The benefits are:

  • All those as the ordering adopted in Changing the default dimension order #8.
  • Most analyses involve reductions over draws or draws and chains, which with column major arrays will generally be fastest with this ordering.
  • Most plots involve slicing first by parameters and then by chains. So this layout is most natural for plotting
  • A simple reshape forms a matrix with (sample, param), which is easily converted to a table of draws and which can be an input to MLJ.

Downsides of this approach:

  • We use a different stacking dimension when combining draws vs combining chains, so we can't just recursively stack. But this is probably safer anyways.
  • Combining draws and chains is in general a little slower. This is just an up-front cost though and is minor.
  • The ordering is not simply the reverse of Python ArviZ's, so when a Python ArviZ user loads a netCDF saved here in Python ArviZ, the dimension order won't match those in Python ArviZ. This isn't a problem, since ArviZ supports all dimension orders.

@sethaxen
Copy link
Member Author

cc @ParadaCarleton

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Merging #40 (ab61f6d) into main (e011ecb) will increase coverage by 0.98%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   96.00%   96.98%   +0.98%     
==========================================
  Files           9        9              
  Lines         300      299       -1     
==========================================
+ Hits          288      290       +2     
+ Misses         12        9       -3     
Impacted Files Coverage Δ
src/dimensions.jl 100.00% <100.00%> (+5.12%) ⬆️
src/from_namedtuple.jl 97.29% <100.00%> (+2.17%) ⬆️
src/utils.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sethaxen
Copy link
Member Author

sethaxen commented Dec 4, 2022

FYI @goedman after this PR is merged, you would need to use permute dimensions using permutedims or PermutedDimsArray in StanSample.

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.

2 participants