BUG: out-of-bounds pydatetime with errors='coerce' and non-ISO format coerces all elements to NaT

[MarcoGorelli]

• closes BUG: out-of-bounds pydatetime with errors='coerce' and non-ISO format coerces all elements to NaT #50255 (Replace xxxx with the GitHub issue number)
• closes BUG: Invalid format to to_datetime with errors='coerce' doesn't raise #50266
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

I haven't added a whatsnew note for #50255 , as this feature (parsing pydatetime / datetime / np.datetime64) objects with non-ISO formats wasn't available to users in the previous release anyway

---

This is much easier to review with "hide whitespace", as many of the changes are just changing a level of indentation

[MarcoGorelli]

the test claims to be testing invalid scalars, but in reality it was erroring before the parsing even started because "H%:M%:S%" is an invalid format

So, I've changed the format to reflect what it looks like this test was meant to be testing, and have made a separate test above to check that invalid formats raise

[MarcoGorelli]

same as above (this errors before even reaching the out-of-bounds scalar...)

[jbrockmendel]

is the code inside this try/except just indenting whats currently there or are there other changes?

[MarcoGorelli]

it's basically just that, but I also removed some

                if is_coerce:
                    iresult[i] = NPY_NAT
                    continue

as that already happens in the outer try-except

If you hide whitespace it'll be easier to see - you can also go to https://github.com/pandas-dev/pandas/pull/50265/files?diff=unified&w=1 (notice the ?diff=unified&w=1 at the end) to see it

[mroeschke]

Generally I'm not a big fan of try/excepts with a huge block of code inside, but I guess since the failures modes here should be made consistent with the broader to_datetime(errors=) keyword I suppose this is fine.

Question, sometime we are issues some ValueErrors internally in this function that will be caught by this try except e.g.

            if iso_year != -1:
                if iso_week == -1 or weekday == -1:
                    raise ValueError("ISO year directive '%G' must be used with "
                                     "the ISO week directive '%V' and a weekday "
                                     "directive '%A', '%a', '%w', or '%u'.")

Is this necessarily a "parsing" error that should be influenced by the errors keyword?

[MarcoGorelli]

I can rewrite it such that each check_dts_bounds is within its own try-except - maybe that's better actually, thanks

I'll get back to you on that ValueError

[MarcoGorelli]

OK, refactored

That's a really good point about the "ISO year directive" error, thanks, good catch! So, I think that one should always raise. At least, that seems to be the current intention, there's no if is_coerce check there like there is for some other errors, such as

pandas/pandas/_libs/tslibs/strptime.pyx

Lines 378 to 382 in a5cbd1e

	except ValueError:
	if is_coerce:
	iresult[i] = NPY_NAT
	continue
	raise

So, I've parametrised test_error_iso_week_year too over errors to check that this one actually raises

[mroeschke]

I am not actually opposed to the large try/except block if we expect ValueError and OutofBounds Errors to be affected by errors. If other excepts in this functions can be factored out (like the iso year directive one) and others then it would help ensure errors is consistently handled

[MarcoGorelli]

OK thanks - have factored out the iso year directive check in #50309, so I've gone back to having an outer try-except block (so again, "hide-whitespace" will come in handy when reviewing)

[MarcoGorelli]

@jbrockmendel @mroeschke sorry for the ping, just wanted to ask if we could get this one in, as:

• it unblocks ERR non-ISO formats don't show position of error #50361
• which in turn unblocks BUG refactor datetime parsing and fix 8 bugs #50242

That would conclude the major to_datetime block of work I was planning to do - anything else would be relatively minor / fixing anything that comes up

Thanks 🙌

[mroeschke]

LGTM