CLN: Using clearer imports #32459
Merged
CLN: Using clearer imports #32459
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbrockmendel
reviewed
Mar 5, 2020
| # calculation and thus could have different value for the day of the wk | ||
| # calculation. | ||
| try: | ||
| if julian == -1: | ||
| # Need to add 1 to result since first day of the year is 1, not | ||
| # 0. | ||
| ordinal = datetime_date(year, month, day).toordinal() | ||
| julian = ordinal - datetime_date(year, 1, 1).toordinal() + 1 | ||
| ordinal = datetime.date(year, month, day).toordinal() |
There was a problem hiding this comment.
this is going to involve an extra python-space lookup, need to check perf
There was a problem hiding this comment.
This is the test case I have used for both of the tests:
import numpy as np
import pandas._libs.tslibs.strptime as strptime
f = "%Y-%m-%d %H:%M:%S %Z"
arr = ['2010-01-01 12:00:00 UTC +0100'] * 100
strptime.array_strptime(np.array(arr, dtype="object"), f, exact=False)master:
In [5]: %timeit strptime.array_strptime(np.array(arr, dtype="object"), f, exact=False)
1.19 ms ± 15.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
PR:
In [5]: %timeit strptime.array_strptime(np.array(arr, dtype="object"), f, exact=False)
1.2 ms ± 13.5 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
|
@MomIsBestFriend in tests.tseries.offsets.test_offset_properties there are a couple of xfailed tests that would be really nice to get fixed. any interest? |
@jbrockmendel Sure! will open a PR soon |
5 tasks
|
the most common usage of datetime import in the codebase is vs. if we want to be consistent, then i'd be happy with standardising on the more common pattern as that would also address #32459 (comment) |
simonjayhawkins
requested changes
Mar 7, 2020
@simonjayhawkins Sure! What are the considerations to use from datetime import foo bar baz
print(foo)
print(bar)
print(baz)over import datetime
print(datetime.foo)
print(datetime.bar)
print(datetime.baz)I think that the second one is clearer as it gives more context. |
|
both styles have pros and cons, and my preference here is mainly due to the prevalence of the first style in our codebase. |
@simonjayhawkins if the main objection is due to the prevalence of the current pattern in the code base, this should not be a large issue, I can fix all of those pretty easily. |
I don't doubt that, but legacy codebases tend to have significant intrinsic value. So the prevalence argument is based on leveraging the experience of the developers before us. |
I see, obviously some parts of the code will be more difficult to tackle than others, but don't you think that we should agree on one specific pattern? (at least for I do think that imports like this, are good from datetime import date as datetime_dateI rather prefer, this more (over the previous one) from datetime import date |
most definitely and datetime is really the issue since it doesn't follow the style PEP with regards to module and object naming. I should of made it clear in #32364 (review) that proper discussion takes place before introducing these changes. IMO when doing cleaning PRs, the patterns used should ONLY be patterns that have been previously discussed/accepted. Otherwise, a precedent tends to be set and the pattern is not adopted through conscious choice. Consistency is important, so this PR should also include updates to the style guide, if we are adopting a new pattern. |
|
@simonjayhawkins what is the PEP import style here that is recommended? |
|
AFAICT both styles are perfectly acceptable and PEP8 doesn't have a preference. my reference to the PEP above refers to datetime naming the module and an object the same. so we also have some cases where we use aliases (not many though) most imports are or the form the google style, however, does recommend the second approach http://google.github.io/styleguide/pyguide.html#22-imports and I am fine with this, as long as sufficient discussion has taken place. (and cython performance is not affected) |
|
Outstanding thoughts on this? Personally OK with it but if there are objections I think OK to just leave as is for now and close PR |
|
i do like this if the perf is not impacted |
|
fine by me |
|
@MomIsBestFriend can you run benchmarks and post results to be sure? |
@WillAyd I have ran a benchmark and posted it in #32459 (comment) Do you want me to merge master and run it again? |
I have no objections to adopting the google style for imports throughout. (would also be happy to adopt the complete google style as a fallback where we don't have a pandas specific recommendation, but that's another discussion) In the style guide we could make the split into Python style guide and Cython style guide more obvious since some python style patterns may not translate well to Cython. IMO having different styles for Python and Cython is acceptable if documented. "A Foolish Consistency is the Hobgoblin of Little Minds".. https://www.python.org/dev/peps/pep-0008/ |
simonjayhawkins
approved these changes
Mar 17, 2020
Can you run the actual ASV benchmarks for tslibs? |
No actually I can not, I never managed to run ASV, I use ASV complains I don't have python 3.6 and each time I solve one complain it has another |
|
You can change the python version in the asv config file |
|
thanks |
SeeminSyed
pushed a commit
to CSCD01-team01/pandas
that referenced
this pull request
Mar 22, 2020
Co-authored-by: MomIsBestFriend <>
jbrockmendel
pushed a commit
to jbrockmendel/pandas
that referenced
this pull request
Mar 23, 2020
Co-authored-by: MomIsBestFriend <>
jbrockmendel
pushed a commit
to jbrockmendel/pandas
that referenced
this pull request
Mar 25, 2020
Co-authored-by: MomIsBestFriend <>
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff