TST: Tests and Helpers for Datetime/Period Arrays #23502
Conversation
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
| @@ -1049,6 +1051,18 @@ def assert_period_array_equal(left, right, obj='PeriodArray'): | |||
| assert_attr_equal('freq', left, right, obj=obj) | |||
|
|
|||
|
|
|||
| def assert_datetime_array_equal(left, right, obj='DatetimeArray'): | |||
| _check_isinstance(left, right, DatetimeArray) | |||
There was a problem hiding this comment.
why is this needed? why don’t we just have a more generic assert_array_equal
adding more comparison routines generally is just asking for trouble - we already have too many
There was a problem hiding this comment.
I think we may be able to de-duplicate these once DTA/TDA are EAs, but for now the options are to have this function or to implement something equivalent inside tm.assert_equal
There was a problem hiding this comment.
I think it is fine to reconsider this once all internal ExtensionArrays are in place (I think the same was said when adding the assert_period_array_equal that is just above this one)
There was a problem hiding this comment.
adding more comparison routines generally is just asking for trouble - we already have too many
I really like having separate ones, so that you can assert that you aren't accidentally comparing two of the wrong kind of object. If you just have a generic assert_array_equal you could have built two ndarrays on accident and there's no way to check that the type is correct (unless you pass that as a parameter?)
There was a problem hiding this comment.
again not sure why this is; the point is we are comparing versus an expected value
so this just extra verbose
eg we recently consolidatednto assert_equal to avoid this exact prome
now adding these back is just more code
sure it’s slightly more explicit but IMHO is not worth the extra functions and maintenance over time
There was a problem hiding this comment.
comparing versus an expected value
sometimes though, that expected value isn't constructed explicitly. Sometimes it's the result of some computation (think of the tests in ops for example).
eg we recently consolidatednto assert_equal to avoid this exact prome
now adding these back is just more code
assert_equal just dispatches to each of the specialized asserts. The code has to live somewhere :) But, I don't mean to nitpick over this. I would just rather type
tm.assert_period_array_equal(a, b)than
tm.assert_array_equal(a, b, kind='period')or, worse, tm.assert_array_equal(a, b), because I'm lazy, and accidentally not have a period array.
So that's my case, but I'm more than happy to be out voted here :)
There was a problem hiding this comment.
it’s fine
just want to reduce maintenance burden
gfyoung
added
Testing
| @@ -1049,6 +1051,18 @@ def assert_period_array_equal(left, right, obj='PeriodArray'): | |||
| assert_attr_equal('freq', left, right, obj=obj) | |||
|
|
|||
|
|
|||
| def assert_datetime_array_equal(left, right, obj='DatetimeArray'): | |||
| _check_isinstance(left, right, DatetimeArray) | |||
There was a problem hiding this comment.
I think it is fine to reconsider this once all internal ExtensionArrays are in place (I think the same was said when adding the assert_period_array_equal that is just above this one)
But do they depend on each other? (meaning, does one need to be merged before the other, or they are orthogonal implementation-wise?) |
They are orthogonal implementation-wise. They are both needed in order to make major progress on sharing existing tests. |
Codecov Report
@@ Coverage Diff @@
## master #23502 +/- ##
==========================================
- Coverage 92.25% 92.25% -0.01%
==========================================
Files 161 161
Lines 51237 51260 +23
==========================================
+ Hits 47269 47290 +21
- Misses 3968 3970 +2
Continue to review full report at Codecov.
|
|
There seemed to be an install error with one of the windows workers. Triggered a rebuild. |
|
rebuild green |
|
Travis failure looks wonky and unrelated |
|
Thanks @jbrockmendel ! |
…fixed * upstream/master: (47 commits) CLN: remove values attribute from datetimelike EAs (pandas-dev#23603) DOC/CI: Add linting to rst files, and fix issues (pandas-dev#23381) PERF: Speeds up creation of Period, PeriodArray, with Offset freq (pandas-dev#23589) PERF: define is_all_dates to shortcut inadvertent copy when slicing an IntervalIndex (pandas-dev#23591) TST: Tests and Helpers for Datetime/Period Arrays (pandas-dev#23502) Update description of Index._values/values/ndarray_values (pandas-dev#23507) Fixes to make validate_docstrings.py not generate warnings or unwanted output (pandas-dev#23552) DOC: Added note about groupby excluding Decimal columns by default (pandas-dev#18953) ENH: Support writing timestamps with timezones with to_sql (pandas-dev#22654) CI: Auto-cancel redundant builds (pandas-dev#23523) Preserve EA dtype in DataFrame.stack (pandas-dev#23285) TST: Fix dtype mismatch on 32bit in IntervalTree get_indexer test (pandas-dev#23468) BUG: raise if invalid freq is passed (pandas-dev#23546) remove uses of (ts)?lib.(NaT|iNaT|Timestamp) (pandas-dev#23562) BUG: Fix error message for invalid HTML flavor (pandas-dev#23550) ENH: Support EAs in Series.unstack (pandas-dev#23284) DOC: Updating DataFrame.join docstring (pandas-dev#23471) TST: coverage for skipped tests in io/formats/test_to_html.py (pandas-dev#22888) BUG: Return KeyError for invalid string key (pandas-dev#23540) BUG: DatetimeIndex slicing with boolean Index raises TypeError (pandas-dev#22852) ...
Implement ABCDatetimeArray, ABCTimedeltaArray
Fix some problems in DatetimeArray comparison methods, with tests
Implement assert_datetime_array_equal, still need to do the same for TimedeltaArray
Extend fixtures to allow us to run some of the arithmetic tests on PeriodArray, DatetimeArray. Change a few usages to use these fixtures.
This is a companion-PR to #23493. Without that one, we can't easily extend the existing tests. Without this one, problems in the comparison methods would cause failures.