Closed OriolAbril closed 8 months ago
I would not break the older versions.
Let's add similar checks as we have other older cmdstanpy stuff
My priority right now is fixing CI so @tomicapretto and the sprint participants can submit PRs and not see everything red. And I don't have a lot of time available so I can't check all cmdstanpy versions to make sure. I do think I have not broken anything though, and all tests do seem to pass now.
Merging #2278 (38cd390) into main (d163000) will increase coverage by
0.46%
. The diff coverage is85.00%
.
@@ Coverage Diff @@
## main #2278 +/- ##
==========================================
+ Coverage 87.87% 88.33% +0.46%
==========================================
Files 122 122
Lines 12459 12442 -17
==========================================
+ Hits 10948 10991 +43
+ Misses 1511 1451 -60
Files Changed | Coverage Δ | |
---|---|---|
arviz/data/io_cmdstanpy.py | 77.18% <85.00%> (+16.54%) |
:arrow_up: |
I will switch off cmdstanpy tests and let's fix this correctly.
sounds good, thanks! it might also be a good idea to start testing this manually on multiple cmdstanpy releases and see how it goes in case it works already
I believe this should work as far back as 0.9.77, which added stan_variables
and method_variables
.
Note that the stan_variables
dictionary isn't cached on the cmdstan side, so these calls here could lead to more computation than you'd expect -- it may be worth storing the keys versus multiple calls, especially since it seems like you mostly care about the names
The changes in this PR work for me. Curious if there's a plan for when this might be folded into a new version of arviz? Having this work out of the box would greatly simplify my cmdstanpy workflow.
I have done some tests locally (expand details tag to see all the results). I did catch an error to trigger the old logic, but there are some versions in between 0.9.77 and 0.9.67 that would require a 3rd logic (so we'd have 3 converters in one). I think this is worth it maintenance wise, nor I think it is worth it to hold this PR.
The converter is currently broken, so unless someone can take over this and fix the converter for all cases we have to choose between supporting the latest cmdstanpy or the 0.9.77-0.9.67 window. I would like to make a new release next week, if we have a better fix I will use that (or wait for that other PR before releasing), otherwise I will merge this PR so we can released the fix for latest cmdstanpy.
Moreover, given that out of these 3 converters (already happens with our 2nd one) we'd only test the newest one, it is quite possible that we might break them without realizing.
Ok, then I would say that we just go forward and support only the latest API.
Is there something we are going to lose compared to older transformer?
If someone requires a CmdStanPy from over two years ago, they are probably also going to be using an older Arviz, to be fair
Yeah, and updating cmdstanpy should be easy and we always have support for cmdstan.
What we probably need to figure out is the tuple parameters (we probably want to unpack them, and also dtype extraction function)
I updated the stanio
package to support numpy custom dtypes for tuples, so cmdstanpy can if we just accept extra kwargs in a few places.
I will close this for now. But I like the changes in tests, maybe we could add them?
Description
Removes all use of the
metadata
object, now only the attribute presence is checked. That is because the attribute is public, but the class returned is part of the private API, so we should not rely on its information or attributes.Fixes #2276, related to https://github.com/stan-dev/cmdstanpy/issues/693.
I might break integration with other older versions, I'll try to check and add a not supported informative error. Older ArviZ should be used for older cmdstanpy.
I have also updated the test infrastructure a bit to make it easier to regenerate the csv files we have in
saved_models
folder. We still use those to avoid installing and running stan for inference, but if the folder is missing/deleted, then the files are regenerated and updated automatically before running the tests.Checklist
:books: Documentation preview :books:: https://arviz--2278.org.readthedocs.build/en/2278/