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

ENH: allow using '+' sign in the argument to `to_offset()` #18171

Merged
merged 3 commits into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@frexvahi
Contributor

frexvahi commented Nov 8, 2017

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@frexvahi

This comment has been minimized.

Contributor

frexvahi commented Nov 8, 2017

I made the change here in frequencies.pxd, which presumably means that now frequencies can also be specified with a '+' sign. Maybe this is not desired, in which case I could include a modified version of the opattern regexp in the to_offset function itself.

@frexvahi

This comment has been minimized.

Contributor

frexvahi commented Nov 8, 2017

Should the whatsnew entry go in "Other API changes"?

@frexvahi

This comment has been minimized.

Contributor

frexvahi commented Nov 8, 2017

Also, does this need a separate issue?

@TomAugspurger

This can go under "Other Enhancements" and you can use 18171 for the issue number. No need for a separate issue.

except Exception:
raise ValueError(_INVALID_FREQ_ERROR.format(freq))
except Exception as e:
raise ValueError(_INVALID_FREQ_ERROR.format(freq)) from e

This comment has been minimized.

@TomAugspurger

TomAugspurger Nov 8, 2017

Contributor

I don't think this is valid syntax in python 2

@@ -169,6 +169,15 @@ def test_to_offset_leading_zero(self):
result = frequencies.to_offset(freqstr)
assert (result.n == -194)
def test_to_offset_leading_plus(self):

This comment has been minimized.

@TomAugspurger

TomAugspurger Nov 8, 2017

Contributor

Could you add a couple invalid freqstrs, like '+-1d', '-+1d', '+1', '+d' and ensure that they all raise.

At least I think they should all raise? d is apparently valid, but -d isn't.

frexvahi added some commits Nov 8, 2017

@codecov

This comment has been minimized.

codecov bot commented Nov 9, 2017

Codecov Report

Merging #18171 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18171      +/-   ##
==========================================
- Coverage   91.42%   91.38%   -0.05%     
==========================================
  Files         163      163              
  Lines       50064    50068       +4     
==========================================
- Hits        45772    45755      -17     
- Misses       4292     4313      +21
Flag Coverage Δ
#multiple 89.19% <100%> (-0.03%) ⬇️
#single 40.33% <0%> (-0.09%) ⬇️
Impacted Files Coverage Δ
pandas/tseries/frequencies.py 96% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.38% <0%> (-1.82%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️
pandas/core/generic.py 95.72% <0%> (ø) ⬆️
pandas/io/formats/format.py 96.01% <0%> (ø) ⬆️
pandas/core/config_init.py 98.34% <0%> (ø) ⬆️
pandas/io/parquet.py 65.38% <0%> (ø) ⬆️
pandas/compat/__init__.py 58.1% <0%> (ø) ⬆️
pandas/core/groupby.py 92.04% <0%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17e0b13...71bd8db. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Nov 9, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@a6345c7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #18171   +/-   ##
=========================================
  Coverage          ?   91.38%           
=========================================
  Files             ?      163           
  Lines             ?    50064           
  Branches          ?        0           
=========================================
  Hits              ?    45751           
  Misses            ?     4313           
  Partials          ?        0
Flag Coverage Δ
#multiple 89.19% <ø> (?)
#single 40.36% <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6345c7...5d0b363. Read the comment docs.

@frexvahi frexvahi changed the title from Allow using '+' sign in the argument to `to_offset()` to ENH: allow using '+' sign in the argument to `to_offset()` Nov 10, 2017

@jreback

small doc change. ping on green.

@@ -23,7 +23,7 @@ Other Enhancements
^^^^^^^^^^^^^^^^^^
- Better support for ``Dataframe.style.to_excel()`` output with the ``xlsxwriter`` engine. (:issue:`16149`)
-
- ``pd.tseries.frequencies.to_offset()`` now accepts '+' signs e.g. '+1h'. (:issue:`18171`)

This comment has been minimized.

@jreback

jreback Nov 10, 2017

Contributor

use a :func:`pandas.tseries.frequencies.to_offset` here

say accepts leading '+' signs

This comment has been minimized.

@frexvahi

frexvahi Nov 10, 2017

Contributor

OK, done

@@ -23,7 +23,7 @@ Other Enhancements
^^^^^^^^^^^^^^^^^^
- Better support for ``Dataframe.style.to_excel()`` output with the ``xlsxwriter`` engine. (:issue:`16149`)
-
- :func:`pd.tseries.frequencies.to_offset()` now accepts leading '+' signs e.g. '+1h'. (:issue:`18171`)

This comment has been minimized.

@jreback

jreback Nov 10, 2017

Contributor

I think this needs to be pandas (and not pd)

This comment has been minimized.

@frexvahi

frexvahi Nov 10, 2017

Contributor

The other entries in the file all say 'pd' and not 'pandas'. In previous whatsnew.txt files there seems to be a mixture but still a preponderance of 'pd'

This comment has been minimized.

@jreback

jreback Nov 10, 2017

Contributor

hmm, then maybe this works. ok.

@jreback jreback merged commit 75c1fa0 into pandas-dev:master Nov 10, 2017

0 of 3 checks passed

ci/circleci Your tests are queued behind your running builds
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Nov 10, 2017

thanks @frexvahi

@frexvahi frexvahi deleted the frexvahi:allow-plus-sign-in-to_offset branch Nov 10, 2017

No-Stream added a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment