-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add to_dict method to InferenceData object #1223
Conversation
Currently implemented
will return:
|
I would have expected
Let's see what everyone else thinks |
@OriolAbril I have done the changes. Have a look! |
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.
of the several possibilities there will be after merging #1201:
- chains and draws equal between post and prior -> custom! i.e.
chain=["A", "B"]
(I would not support that) - chains and draws equal between post and prior, index_origin=0 (this is not an issue now, will be once we merge labeling restructure
- chains and draws equal between post and prior, index_origin=1 (as above)
- different chains and draws
some cases could be solved by adding chain and draw dims in resulting dict, but not all of them, there should be some kind of check on index_origin
and it be returned. Here are some examples:
- case 1: index_origin=0
- posterior
- chan: 0, 1, 2, 3
- draw: np.arange(100)
- prior
- chain: 0
- draw: np.arange(200)
- posterior
- case 2: index_origin=1
- posterior
- chan: 1, 2, 3, 4
- draw: np.arange(1, 101)
- prior
- chain: 1
- draw: np.arange(1, 201)
- posterior
Codecov Report
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
+ Coverage 91.75% 91.79% +0.03%
==========================================
Files 101 101
Lines 10568 10622 +54
==========================================
+ Hits 9697 9750 +53
- Misses 871 872 +1
Continue to review full report at Codecov.
|
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.
The number of arguments in from_dict
is starting to become challengingly large, shoud it be modified to take kwargs and call dict_to_dataset
for all of them (checking for observed_data, constant_data to set skip_dims=[]
)?
This would allow users to introduce any group which may not be ideal but would make code quite easier to follow and maintain.
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.
I think we can start with tests on this one.
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.
LGTM
Are we saving attr to all groups? I''m fine with this, it is not optimal, but people handling with large attrs info should manually handle the attrs.
We currently store the version, time, ppl library as attrs to all groups, whereas things like sampling time is stored only in posterior or sample_stats |
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.
I think it's ready to merge, only one small nit
Hi, remember to |
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.
LGTM, @ahartikainen you can merge when you see this provided that test have passed
Description
This PR adds the
to_dict
method to InferenceData object.Checklist