Skip to content
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

Can't get period code with frequency alias 'minute' or 'Minute' #11854

Closed
JCalderan opened this issue Dec 16, 2015 · 3 comments
Closed

Can't get period code with frequency alias 'minute' or 'Minute' #11854

JCalderan opened this issue Dec 16, 2015 · 3 comments
Labels
Bug Frequency DateOffsets
Milestone

Comments

@JCalderan
Copy link

Hi,

While playing with the frequency module, I might have found a bug with the function _period_str_to_code:
the function didn't return the code 8000 for the frequency string "minute", yet it works for frequency strings as 'T', 'Min', 'min'...

Here's the code to reproduce the 'bug':

>>> from pandas.tseries.frequencies import _period_str_to_code
>>> _period_str_to_code('Min')
8000
>>> _period_str_to_code('T')
8000
>>> _period_str_to_code('minute')
sys:1: FutureWarning: Freq "MINUTE" is deprecated, use "Min" as alternative.
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "***/site-packages/pandas/tseries/frequencies.py", line 813, in _period_str_to_code
    return _period_code_map[alias]
KeyError: 'Min'

(in pandas/tseries/frequencies.py line 783 to line 813)
It appears that 'minute' is converted into the alias 'Min' during the function __period_str_to_code_ (line 807).
'Min' is then used as a key in the dictionnary __period_code_map, but this dictionnary doesn't hold any value for this key (line 813).
Indeed, the key 'Min' is stored in the dictionnary __lite_rule_alias
where it indexes the value 'T' (the correct frequency string for minutes).

This particuliar situation could be solved by transforming the return of the function __period_str_to_code_ by using a recursive call which allows to 'safely' handle the converted frequency string (_period_str_to_code('minute') > 'Min' > _period_str_to_code('Min') > 8000).
The conversion from 'Min' to 'T' occures line 799, and the conversion from 'T' to 8000 occures line 804.

But I'm not sure of the implication of such a recursive call (performance impact, possibility of an 'infinite' recursive call in some situation, and so on).
Here is the function with a recursive call (change in pandas/tseries/frequencies.py line 813):

def _period_str_to_code(freqstr):
    # hack
    if freqstr in _rule_aliases:
        new = _rule_aliases[freqstr]
        warnings.warn(_LEGACY_FREQ_WARNING.format(freqstr, new),
                      FutureWarning, stacklevel=3)
        freqstr = new
    freqstr = _lite_rule_alias.get(freqstr, freqstr)

    if freqstr not in _dont_uppercase:
        lower = freqstr.lower()
        if lower in _rule_aliases:
            new = _rule_aliases[lower]
            warnings.warn(_LEGACY_FREQ_WARNING.format(lower, new),
                          FutureWarning, stacklevel=3)
            freqstr = new
        freqstr = _lite_rule_alias.get(lower, freqstr)

    try:
        if freqstr not in _dont_uppercase:
            freqstr = freqstr.upper()
        return _period_code_map[freqstr]
    except KeyError:
        try:
            alias = _period_alias_dict[freqstr]
            warnings.warn(_LEGACY_FREQ_WARNING.format(freqstr, alias),
                          FutureWarning, stacklevel=3)
        except KeyError:
            raise ValueError("Unknown freqstr: %s" % freqstr)

        return _period_str_to_code(alias)
@jreback
Copy link
Contributor

jreback commented Dec 16, 2015

cc @sinhrks

@jreback jreback added the Frequency DateOffsets label Dec 16, 2015
@sinhrks
Copy link
Member

sinhrks commented Mar 5, 2016

Should we change this to T? Others are all mapped to single character representation.

https://github.com/pydata/pandas/blob/master/pandas/tseries/frequencies.py#L708

@sinhrks sinhrks added the Bug label Mar 5, 2016
@jreback
Copy link
Contributor

jreback commented Mar 6, 2016

that seems right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Frequency DateOffsets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants