-
Notifications
You must be signed in to change notification settings - Fork 137
Fix mean, var and std for XTensorVariables #1533
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
Conversation
d89351c
to
33e831a
Compare
Sorry for the noise, should have checked :l |
No worries |
e43020c
to
3a5782b
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.
left a comment about the output doctest directive, ready to merge after addressing
pytensor/xtensor/type.py
Outdated
out = x[:, idx].set(1) | ||
out.eval() | ||
.. test-output:: |
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.
The output version is also without hyphen: https://www.sphinx-doc.org/en/master/usage/extensions/doctest.html#directive-testoutput. It is also hidden from the rendered output due to unrecognized directive for now
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.
Is there no pre-commit we could use to fail on wrong directives?
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.
There is the "fail on warning" rtd flag which would fail on wrong directives, things missing from toctree. If there are only a handful of warnings I can fix all the existing ones and set this up. Not sure about directives, I think I saw an rST linter once, but directives can be custom so not sure if it would catch that or simply focus on the syntax of the directve instead of the content
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.
Feel free to check if number of warnings is crazy, that would be useful even if it doesn't address my problem
f9b6405
to
c8f9c74
Compare
Also remove broadcast which is not a method in Xarray
c8f9c74
to
6ab3cbd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
+ Coverage 81.86% 81.90% +0.04%
==========================================
Files 230 230
Lines 52508 52506 -2
Branches 9339 9339
==========================================
+ Hits 42984 43006 +22
+ Misses 7092 7070 -22
+ Partials 2432 2430 -2
🚀 New features to boost your workflow:
|
Also add docstrings to most methods and properties so they show up in the docs.
Removes broadcast as a method, since it is not one in xarray
📚 Documentation preview 📚: https://pytensor--1533.org.readthedocs.build/en/1533/