arviz icon indicating copy to clipboard operation
arviz copied to clipboard

Make `from_dict` more flexible, and add `from_pytree`

Open ColCarroll opened this issue 2 years ago • 8 comments

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.


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

ColCarroll avatar Nov 14 '23 20:11 ColCarroll

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).

ColCarroll avatar Nov 14 '23 20:11 ColCarroll

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

ahartikainen avatar Nov 14 '23 21:11 ahartikainen

It looks like maybe: https://github.com/google-deepmind/tree/issues/109

I gave https://github.com/google-deepmind/tree/pull/111 a try.

ColCarroll avatar Nov 14 '23 21:11 ColCarroll

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 :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 15 '23 03:11 codecov[bot]

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 avatar Nov 15 '23 15:11 ColCarroll

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

ColCarroll avatar Jan 25 '24 15:01 ColCarroll

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

ColCarroll avatar Mar 11 '24 16:03 ColCarroll

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

I think this is ready again.

ColCarroll avatar Mar 13 '24 12:03 ColCarroll

if @OriolAbril approves I think we can merge

ahartikainen avatar Mar 13 '24 15:03 ahartikainen

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

OriolAbril avatar Mar 13 '24 16:03 OriolAbril

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.

ColCarroll avatar Mar 13 '24 18:03 ColCarroll

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

OriolAbril avatar Mar 13 '24 19:03 OriolAbril

LGTM!

On Wed, Mar 13, 2024, 3:16 PM Oriol Abril-Pla @.***> wrote:

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

Captura.de.pantalla.de.2024-03-13.19-42-15.png (view on web) https://github.com/arviz-devs/arviz/assets/23738400/35f4b904-ac37-4805-bb52-37dcf6439f90

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

— Reply to this email directly, view it on GitHub https://github.com/arviz-devs/arviz/pull/2291#issuecomment-1995455446, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARQOEH3YJBX5ZB3GZQF5ILYYCQXHAVCNFSM6AAAAAA7LNJR62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJVGQ2TKNBUGY . You are receiving this because you authored the thread.Message ID: @.***>

ColCarroll avatar Mar 13 '24 23:03 ColCarroll

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.

papoteur-mga avatar May 17 '24 10:05 papoteur-mga

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.

ColCarroll avatar May 23 '24 17:05 ColCarroll

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.

papoteur-mga avatar May 24 '24 07:05 papoteur-mga

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!)

ColCarroll avatar May 24 '24 15:05 ColCarroll