... #25315
... #25315
Conversation
|
Please add test(s) as they should always come first |
| if ddof == 1: | ||
| try: | ||
| return self._cython_agg_general('std', **kwargs) | ||
| except Exception: |
There was a problem hiding this comment.
Not entirely clear what this is for but except for legacy code we never catch just base Exception so will need something more explicit
There was a problem hiding this comment.
Copy / paste isn't the right way to go here. Can you still keep the dispatch to self.var and just see what is mangling the expression once passed as an argument to np.sqrt instead?
Codecov Report
@@ Coverage Diff @@
## master #25315 +/- ##
===========================================
- Coverage 91.72% 41.72% -50.01%
===========================================
Files 173 173
Lines 52831 52840 +9
===========================================
- Hits 48457 22045 -26412
- Misses 4374 30795 +26421
Continue to review full report at Codecov.
|
| # 1 b B 1.414214 | ||
|
|
||
| assert_series_equal( | ||
| np.sqrt(group_var["X"]), |
There was a problem hiding this comment.
Unless there is a reason for using np.sqrt here just explicitly construct the expected values in a variable called expected. Also do result = on the groupby op you are testing and simply make this line assert_series_equal(result, expected) (should see this usage throughout tests if unclear from this comment)
| if ddof == 1: | ||
| try: | ||
| return self._cython_agg_general('std', **kwargs) | ||
| except Exception: |
There was a problem hiding this comment.
Copy / paste isn't the right way to go here. Can you still keep the dispatch to self.var and just see what is mangling the expression once passed as an argument to np.sqrt instead?
|
@WillAyd comments here are typical comments for a PR; we require certain a specific style for code and tests as well as maintainable code; catching generic exceptions does not help. if you would like to update pls do so. |
...