Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TST: Tests and Helpers for Datetime/Period Arrays #23502
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
TST: Tests and Helpers for Datetime/Period Arrays #23502
Changes from all commits
083135d
b282e8f
1e56627
5e1b3ef
d1188ac
caedb95
5a02923
cd060d1
baad2af
87070a2
72a849b
03f5208
59b7064
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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 typethan
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it’s fine
just want to reduce maintenance burden