-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add experimental dims
module with objects that follow dim-based semantics (like xarray)
#7820
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
base: main
Are you sure you want to change the base?
Conversation
day_of_conception = datetime.date(2025, 6, 17) | ||
day_of_last_bug = datetime.date(2025, 6, 17) | ||
today = datetime.date.today() | ||
days_with_bugs = (day_of_last_bug - day_of_conception).days |
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.
wtf 😆
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.
This has two purposes: distract reviewers so they don't focus on the critical changes, and prove that OSS libraries can't be fun.
1cfde5b
to
f571e5d
Compare
Can this index using labels? |
f571e5d
to
56402aa
Compare
I don't know what Is Like in xarray, you can do You cannot do The new PyTensor objects have dims but not coords. It's not trivial to encode coord based operations in our backends. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
3eb6738
to
67a0eda
Compare
3234468
to
d4017fb
Compare
We should make this the 6.0 release. |
I agree, but would perhaps wait until we beta-tested it to the point it no longer feels too experimental |
dims
module with objects that follow dim-based semantics (like xarray)
f1f7478
to
56c05f1
Compare
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.
adding one extra comment about myst syntax but outside reviewnb because it always messes up rendering of backticks.
I will try to play around and build/port some models one of these days, and look at the code itself while I do that
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.
Inside the notebook you have to use myst syntax, so you'll have to replace the :reftype:`reftarget`
with {reftype}`reftarget`
@@ -0,0 +1,1666 @@ | |||
{ |
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 this is too high up in the notebook as I envision this as something beginner users should not do, but I think it would be nice to show how to get array values for an XTensorVariable into their corresponding DataArray, the xr.DataArray(pm.draw(...), dims=outer_addition.type.dims)
. Maybe this could go into a dims FAQ? It could be started at the bottom of this page and if we see it grows extend it to their own page.
Or start a global FAQ page with whichever questions/answers in the discord post are still relevant and add a pymc.dims
section. Feel free to make an issue for this to keep the PR scope clear.
Reply via ReviewNB
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.
Good idea, I think we can sneak it in at the end where I mention coordinates. Add a "why are the variables not producing DataArrays directly" with this recipe
@@ -0,0 +1,1666 @@ | |||
{ |
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.
minor nits, it currently says isel
won't be supported instead of sel
. for the link to the dist method to work once we add api docs for the module it needs to be :meth:
Reply via ReviewNB
@@ -0,0 +1,1666 @@ | |||
{ |
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.
Add a tilde before pytensor.xtensor...
(still inside the tildes) so it is rendered as values
only instead of the full name. We should also double check the attribute is documented explicitly in the pytensor docs, otherwise this won't be rendered as a cross-reference
Reply via ReviewNB
@@ -0,0 +1,1666 @@ | |||
{ |
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.
Given we have just looked at the number of nodes as a measure of model complexity/efficiency. I think we should show that to for the dims_splines_model
to show it is roughly the same as the vectorized model, but much easier to read, and (arguably?) easier to code too
Reply via ReviewNB
@@ -0,0 +1,1666 @@ | |||
{ |
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 am skipping this last part for now to see if we converge on the custom object dim approach on pytensor which should make this a non-issue
Reply via ReviewNB
116f803
to
a4273c0
Compare
…h specific lazy imports
Also remove commented out code
* Use RV Op name when provided * More robust detection of observed data variables (after pymc-devs#7656 arbitrary graphs are allowed) * Remove self loops explicitly (closes pymc-devs#7722)
a4273c0
to
412e4df
Compare
This builds on top of PyTensor xtensor module, to introduce distributions and model objects that follow xarray-like semantics. Example model:
Equivalently, with the traditional API:
More details in the new core notebook