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

Add attr and name to idata #1357

Merged
merged 16 commits into from
Jan 16, 2022
Merged

Add attr and name to idata #1357

merged 16 commits into from
Jan 16, 2022

Conversation

percygautam
Copy link
Contributor

@percygautam percygautam commented Aug 20, 2020

Description

Related to the discussion being held in the channel.

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@percygautam
Copy link
Contributor Author

percygautam commented Aug 20, 2020

How these changes are incorporated in concat. Also, please briefly explain the name attribute?

@ahartikainen
Copy link
Contributor

For concat, either we make everything a list or list only when needed, same for name.

idata1.attrs["a"] = 1
idata2.attrs["a"] = 1

-> idata["a"] == [1, 1] or idata["a"] == 1

idata1.attrs["a"] = 1
idata2.attrs["a"] = 2

-> idata["a"] == [1, 2]

@ahartikainen
Copy link
Contributor

I think that might be the simplest option?

@percygautam
Copy link
Contributor Author

I have added attrs to concat as suggested. I was modifying from_dict to include attrs, but it already contains attrs argument, that point to dataset.attrs. Should I change already existing attrs to dataset_attrs? Or something else?

@ahartikainen
Copy link
Contributor

I think attrs in from_dict should point to the main attrs (this what you add right now)

@percygautam
Copy link
Contributor Author

Will this not change the existing API? Also, if we do so, what should the attrs be named? (Maybe group_attrs or dataset_attrs?)

@ahartikainen
Copy link
Contributor

I think existing api is not yet released?

That sounds a good idea.

We could also add kwargs for from_dict and then get attrs for each group if needed?

So attrs could then mean the idata attrs. And each group might have {groupname}_attrs?

@OriolAbril
Copy link
Member

Another question is whether we should modify current code to store arviz/ppl version... as idata attrs instead of repeating them in every group.

@percygautam percygautam marked this pull request as ready for review August 31, 2020 16:29
@ahartikainen
Copy link
Contributor

Do we want to get this in the next release?

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #1357 (fd5c4c4) into main (b54e022) will decrease coverage by 0.04%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1357      +/-   ##
==========================================
- Coverage   90.90%   90.85%   -0.05%     
==========================================
  Files         113      113              
  Lines       12217    12274      +57     
==========================================
+ Hits        11106    11152      +46     
- Misses       1111     1122      +11     
Impacted Files Coverage Δ
arviz/data/inference_data.py 83.02% <88.23%> (-0.53%) ⬇️
arviz/data/io_dict.py 94.07% <100.00%> (+0.68%) ⬆️

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 b54e022...fd5c4c4. Read the comment docs.

@ahartikainen
Copy link
Contributor

This PR can wait to next release. It will change many io functions.

Base automatically changed from master to main January 26, 2021 19:44
@ahartikainen ahartikainen merged commit 4544dd0 into arviz-devs:main Jan 16, 2022
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