REF: strictness and checks for Timedelta _simple_new #23433
Conversation
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
gfyoung
added
Dtype Conversions
| assert isinstance(values, np.ndarray), type(values) | ||
| if values.dtype == 'i8': | ||
| values = values.view('m8[ns]') | ||
| assert values.dtype == 'm8[ns]', values.dtype |
There was a problem hiding this comment.
Same questions for all your assert statements:
- Are these internal? If not, it would be nice to have user-friendly error messages.
- Can these assert statements be tested in any way?
There was a problem hiding this comment.
The question of testing is actually more general to all of these changes. Even though it's been labeled as internal, not sure if any of these edits will surface in any way.
There was a problem hiding this comment.
The edits in simplenew are thoroughly internal; users shouldn’t get near it.
Not sure what testing these assertions would look like. Can you elaborate what you have in mind?
(moving your responses to the conversation bubble in the UI, organizational thing)
@jbrockmendel : What I was wondering was whether we could trigger these assert statements (e.g. an invalid input to a publicly facing function or method)?
Might be tricky if these edits are purely internal, and if it is too difficult, not a big deal. Just out of curiosity since tests are good if we can have them.
There was a problem hiding this comment.
I see. Yah, we could
Write tests where we directly pass invalid inputs to simplenew. (Typing with thumbs, feel free to reformat if necessary)
Codecov Report
@@ Coverage Diff @@
## master #23433 +/- ##
==========================================
- Coverage 92.22% 92.21% -0.02%
==========================================
Files 161 161
Lines 51187 51188 +1
==========================================
- Hits 47209 47202 -7
- Misses 3978 3986 +8
Continue to review full report at Codecov.
|
| @@ -166,6 +167,19 @@ def __new__(cls, data=None, unit=None, freq=None, start=None, end=None, | |||
| elif copy: | |||
| data = np.array(data, copy=True) | |||
|
|
|||
| data = np.array(data, copy=False) | |||
| if data.dtype == np.object_: | |||
| data = array_to_timedelta64(data) | |||
There was a problem hiding this comment.
why are these checks NOT done in _simple_new? this is inconsistent with other code.
We should be really really clear on what is acceptable in _simple_new vs. what is not. IIRC from another of your PR's you did checks on object type in _simple_new for example.
There was a problem hiding this comment.
AFAICT the current verbose-checking is largely driven by the weird cases (that these PRs get rid of) where None or [] is passed to _shallow_copy.
This and the associated DatetimeIndex PR impose a simple/strict API for _simple_new: it expects an np.ndarray that may be either i8 or M8[ns]/m8[ns].
| @@ -131,6 +131,10 @@ def __new__(cls, values, freq=None): | |||
|
|
|||
| freq, freq_infer = dtl.maybe_infer_freq(freq) | |||
|
|
|||
| values = np.array(values, copy=False) | |||
There was a problem hiding this comment.
so why do you need to accept object type here (you are also checking for this in TDI.new). ?
There was a problem hiding this comment.
ATM we are checking for it in TimedeltaArray._simple_new, so de-facto accepting it in TimedeltaArray.__new__. This is leaving the effective __new__ policy unchanged while clearing up the _simple_new policy
There was a problem hiding this comment.
ok, assume this is on the list to de-duplicate
|
Travis failure is for (new?) linting in scripts/docs |
|
Gentle ping, I’m hoping to make a big push on datetime/timedelta array this weekend, getting the constructors locked down is the first step. |
|
ok merging. |
…xamples * repo_org/master: (66 commits) CLN: doc string (pandas-dev#23469) DOC: Add cookbook entry for triangular correlation matrix (GH22840) (pandas-dev#23032) add number of Errors, Warnings to scripts/validate_docstrings.py (pandas-dev#23150) BUG: Allow freq conversion from dt64 to period (pandas-dev#23460) ENH: Add FrozenList.union and .difference (pandas-dev#23394) REF: cython cleanup, typing, optimizations (pandas-dev#23464) strictness and checks for Timedelta _simple_new (pandas-dev#23433) Fixing flake8 problems new to flake8 3.6.0 (pandas-dev#23472) DOC: Updating the docstring of Series.dot (pandas-dev#22890) TST: Fixturize series/test_analytics.py (pandas-dev#22755) BUG/ENH: Handle NonexistentTimeError in date rounding (pandas-dev#23406) PERF: speed up concat on Series by making _get_axis_number() a classmethod (pandas-dev#23404) REF: Remove DatetimelikeArrayMixin._shallow_copy (pandas-dev#23430) REF: strictness/simplification in DatetimeArray/Index _simple_new (pandas-dev#23431) REF: cython cleanup, typing, optimizations (pandas-dev#23456) TST: tweak Hypothesis configuration and idioms (pandas-dev#23441) BUG: fix HDFStore.append with all empty strings error (GH12242) (pandas-dev#23435) TST: Skip 32bit failing IntervalTree tests (pandas-dev#23442) BUG: Deprecate nthreads argument (pandas-dev#23112) style: fix import format at pandas/core/reshape (pandas-dev#23387) ...
Broken off from #23426.
git diff upstream/master -u -- "*.py" | flake8 --diff