pd.Timestamp constructor ignores missing arguments
State: closed

Hello. The PR fixes https://github.com/pandas-dev/pandas/issues/31930

Please review and merge if you think it is good enough :)

Regards,
Artyom.


Labels:
name: Datetime
description: Datetime data dtype
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
Hello @ArtyomKaltovich! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

* In the file [`pandas/tests/scalar/timestamp/test_timezones.py`](https://github.com/pandas-dev/pandas/blob/aebd621c51634570ccb90db103d7db82d1cf07c3/pandas/tests/scalar/timestamp/test_timezones.py):

> [Line 446:89](https://github.com/pandas-dev/pandas/blob/aebd621c51634570ccb90db103d7db82d1cf07c3/pandas/tests/scalar/timestamp/test_timezones.py#L446): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (119 > 88 characters)


Some tests failed due to changes, mostly pickle ones.
I think that tz should be key only parameter (btw, usually it is used in such way), but it causes failures.
What should I do? Change or not to change?)
> 
> 
> #31930 is more about raising when illegal combinations of arguments are specified together (not just tz)
> 
> I think there needs to be more discussion on the issue before we move forward with a solution.

Hello @mroeschke 
Thank you for your feedback.

I've just faced with this bug recently at my job and decided to fix it. But you are right, it is more connected to the issue https://github.com/pandas-dev/pandas/issues/31929

What if the other prohibited combinations? I can name only this one and ts_input with year, month and day specified via key params. I can add this check if it is not yet presented. Are there any more cases? functools.singledispatch could help us here, but it will probably slow down the execution. So I guess there is no better way here except the current one (I mean isinstance checking it the constructor body). Don't you agree?

If you need a little bit more time to think about refactoring, can we for now submit it as a fix for  https://github.com/pandas-dev/pandas/issues/31929 ? Because, as I said previously it prevents me to implement work tasks and it is unexpected behavior what is good to avoid.

I've just need to know if changing tz to key only param is acceptable or not?

Thank you and sorry for mentioning the wrong issue :)

Regards,
Artyom.
I proposed https://github.com/pandas-dev/pandas/issues/31930#issuecomment-585354511 as the only legal combination of parameters for #31930. Probably needs more discussion on that front.

As for #31929 which you are trying to address, rearranging the arguments in the constructor would be an API change and would need to undergo a deprecation cycle. The _easier_ solution in my option would be to deprecate the `tz` argument entirely (which would still require a deprecation cycle)
Ok, I see your point.

So for now, I can try to bypass the bug without changing api. That will fix the problem and #31929  could be closed, while we can still think about better approach for  #31930 


Actually `singledispatch` works with functions with different amount of parameters.

    from datetime import datetime
    from functools import singledispatch
    
    
    @singledispatch
    def Timestamp(tsinput):
        raise NotImplementedError('Unsupported type')
    
    
    @Timestamp.register(int)
    def _(year, month, day):
        print(f"Timestamp({year}, {month}, {day})")
    
    
    @Timestamp.register(str)
    def _(ts_info):
        print(f"Timestamp({ts_info})")
    
    
    @Timestamp.register(datetime)
    def _(ts_info):
        print(f"Timestamp({ts_info})")

    if __name__ == '__main__':
        Timestamp(2020, 2, 2)  # if omit the last arg  
                               # TypeError: _() missing 1 required positional argument: 'day'
                               # will be generated.
        Timestamp("2019-11-11")
        Timestamp(datetime(2017, 3, 4))

produces 

    Timestamp(2020, 2, 2)
    Timestamp(2019-11-11)
    Timestamp(2017-03-04 00:00:00)

but of course more research of performance, function wrupping and doc generation are needed.
Do you want to try to address #31930 with `singledispatch` in this PR @ArtyomKaltovich?
> Do you want to try to address #31930 with `singledispatch` in this PR @ArtyomKaltovich?

If it is acceptable variant, why not?)

But I am not familiar with Pandas CI performance testing, where can I read/know about it?
> i don't think we can use singledispatch in cython (well we can but its going to be non-performant); you can try to prove me wrong though.
> 
> In any event, we simply want to validate combinations of parameters and reject invalid ones.

Hello @jreback 
Thank you for your feedback.
I am also afraid of performance issue, but maybe singledispatch create the same code with isinstance as already presented one. That's why I've asked how can I measure it with Pandas performance testing?

> we cannot change the position of these args
Thank you I will revert parameter order. I can submit a fix for #31929 independently while we a thinking about better parameter validation approach.
Hello @mroeschke @jreback
I was trying to leave param order as is. The following code works:
    
        stamp = Timestamp(2019, 1, 2, 3, 4, 5, 6, 7, tzinfo=pytz.timezone("UTC"))
        stamp = Timestamp(year=2019, month=1, day=2, hour=3, minute=4, second=5, microsecond=6, nanosecond=7, tz="UTC")

but it still has problems with such code:

            stamp = Timestamp(2019, 1, 1, tz="EST5EDT")
            E   TypeError: __new__() got multiple values for keyword argument 'tz'

Because tz is the third arg and 1 is passed as tz, not day.

I can create PR, because it fixes at least one problem, or we can continue to experiment, but I do not see any ways to fix it except singledispatch or parameter reordering :(

Regards.
In terms of performance testing see: https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#running-the-performance-test-suite

I think these issues can be solved together by either reordering parameters or using singledispatch
Thank you @mroeschke 
I will investigate it.
@ArtyomKaltovich do you have time to merge in master and address the review comments?
Hello @mroeschke 
Unfortunately I didn't have enough time to work on it due to work and study, I am planing to do it on this week.

Regards.
closing as stale
if u want to continue pls ping