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

Make from_dict more flexible, and add from_pytree #2291

Merged
merged 4 commits into from
Mar 14, 2024
Merged

Make from_dict more flexible, and add from_pytree #2291

merged 4 commits into from
Mar 14, 2024

Conversation

ColCarroll
Copy link
Member

@ColCarroll ColCarroll commented Nov 14, 2023

This also makes from_dict work for nested dictionaries. I joined the keys with double underscores because using a . would break attribute access (like, it would still work, but you'd use idata.posterior['var.x.y'], instead of idata.posterior.var__x__y)

Hopefully this is a no-op for most code, but it should make arviz work automatically with namedtuples or dataclasses.


📚 Documentation preview 📚: https://arviz--2291.org.readthedocs.build/en/2291/

@ColCarroll
Copy link
Member Author

I'll add that this adds a requirement on dm-tree. I'd rather have used jax.tree_utils, but that requires all of jax (~1.4MB) instead of just dm-tree (~35kb).

@ahartikainen
Copy link
Contributor

Will this affect python 3.12 use? Probably not(?)

@ColCarroll
Copy link
Member Author

It looks like maybe: google-deepmind/tree#109

I gave google-deepmind/tree#111 a try.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: Patch coverage is 96.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.80%. Comparing base (3fc5962) to head (bd03a88).
Report is 2 commits behind head on main.

Files Patch % Lines
arviz/data/converters.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2291   +/-   ##
=======================================
  Coverage   86.79%   86.80%           
=======================================
  Files         123      123           
  Lines       12722    12743   +21     
=======================================
+ Hits        11042    11061   +19     
- Misses       1680     1682    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ColCarroll
Copy link
Member Author

I tried it out, and it fails when trying to install dm-tree on python 3.12, because it requires CMake. Once you conda install CMake and install xcode on OSX, you can install from source, but that's probably too big of a lift for users, and we should wait on this until dm-tree builds a wheel.

Given expected workloads in arviz, we might just write our own flatten_with_path function in pure python. I'll think about that if there's no motion on the other PR.

@ColCarroll
Copy link
Member Author

Update: The PR was merged for dm-tree, but we need to wait on a pypi release.

@ColCarroll
Copy link
Member Author

Hey! They sneakily added PyPI wheels for 3.12 to dm-tree. I think this is now ready to go.

arviz/data/base.py Outdated Show resolved Hide resolved
arviz/tests/base_tests/test_data.py Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ColCarroll ColCarroll force-pushed the pytrees branch 4 times, most recently from 3a563e0 to f6624ee Compare March 13, 2024 12:46
@ColCarroll
Copy link
Member Author

I think this is ready again.

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

It complains about black, so maybe that should be run first

@ahartikainen
Copy link
Contributor

if @OriolAbril approves I think we can merge

@OriolAbril
Copy link
Member

I want to make a patch release before merging, maybe a couple doc changes, then I'll merge.

@ColCarroll
Copy link
Member Author

Sounds good!

bayeux will probably pin to the next release that has this in it, since this example produces a dictionary of parameters that the current az.from_dict fails to parse correctly.

@OriolAbril
Copy link
Member

I think it should be done now. Docs used to look like this:

Captura de pantalla de 2024-03-13 19-42-15

Now they are this: https://arviz--2291.org.readthedocs.build/en/2291/api/generated/arviz.dict_to_dataset.html

@ColCarroll
Copy link
Member Author

ColCarroll commented Mar 13, 2024 via email

@OriolAbril OriolAbril merged commit 29ca5a1 into main Mar 14, 2024
11 checks passed
@OriolAbril OriolAbril deleted the pytrees branch March 14, 2024 00:07
@papoteur-mga
Copy link

Hello,
This is not a cool dependency for Linux distro. Building dm-tree from source needs to download at build time pybind11. However, distros prohibit any download at build time, thus the build fails. The build does not use a library provided by the distro.

@ColCarroll
Copy link
Member Author

Hi @papoteur-mga -- thanks for chiming in, but I'm not sure what this means!

  • Which Linux distro?
  • Why is a linux distro building Python libraries from source?
  • Is dm-tree impossible to use there?
  • Is this really a dm-tree issue?
  • Is there a different way we could achieve this functionality. I know we could depend on jax and use jax.tree, but that seems like a heavier dependency.

@papoteur-mga
Copy link

Hi @papoteur-mga -- thanks for chiming in, but I'm not sure what this means!

* Which Linux distro?

Mageia

* Why is a linux distro building Python libraries from source?

Python packages are included in the distro to be easily installed, in particular as dependency of any application. The most of the packages are tagged as being open source. These packages should then be built to be sure there are really open source. This is also a way to minimize the risk that faulty code is introduced.

* Is `dm-tree` impossible to use there?

* Is this really a `dm-tree` issue?

Yes, but I finally found a way by patching the cmake configuration files in dm-tree to use shared libraries. https://svnweb.mageia.org/packages/cauldron/python-dm-tree/current/SOURCES/0001-use-system-libraries.patch?view=markup

* Is there a different way we could achieve this functionality. I know we could depend on `jax` and use `jax.tree`, but that seems like a heavier dependency.

I have no idea about that.

@ColCarroll
Copy link
Member Author

Thank you for the response! Is it right to say you're here to advocate for mageia users, and not (specifically) as a user of statistical diagnostics?

I have a particular interest in the functionality that dm-tree provides, since it allows for running diagnostics in some of my own work with bayesian neural nets.

It sounds like maybe this is all set from the patch you added? If it isn't, I can try to explore using some other library to achieve my goals (but I suspect it may introduce more/different problems!)

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.

4 participants