Skip to content

DOC: updated documentation for BusinessHour and BusinessDay #50240

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

Merged
merged 6 commits into from
Dec 17, 2022

Conversation

natmokval
Copy link
Contributor

This PR is related to PR #50182.

Updated documentation for BusinessDay, BusinessHour, CustomBusinessHour. Corrected lists of parameters and provided more examples.
Could you, please, @MarcoGorelli, review my PR?

@MarcoGorelli MarcoGorelli self-requested a review December 13, 2022 21:17
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @natmokval ! Got some minor comments, but overall this is a nice improvement

start : str, time, or list of str/time, default "09:00"
Start time of your custom business hour in 24h format.
end : str, time, or list of str/time, default: "17:00"
End time of your custom business hour in 24h format.

Examples
--------
>>> from datetime import time
You can use the parameter ``n`` to represent the shift to n hours.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps "to represent a shift of n hours" would be clearer? likewise for above

>>> ts + pd.offsets.BusinessHour(end=dt_time(19, 0))
Timestamp('2022-08-08 10:00:00')

The parameter ``normalize`` equal True forces shift to midnight.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clearer how you wrote it above

Passing the parameter ``normalize`` equal True, you shift the start
of next business hour to midnight.

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 14, 2022
@natmokval
Copy link
Contributor Author

Thanks for working on this @natmokval ! Got some minor comments, but overall this is a nice improvement

Thank you @MarcoGorelli, I like doing it. I find the idea of slightly improving the documentation of offsets reasonable.

Agreed, I did copy-and-paste this example from CustomBusinessHour. The PR is updated as you suggested.

You can divide your business day hours into several parts.

>>> import datetime as dt
>>> freq = pd.offsets.CustomBusinessHour(start=["06:00", "10:00", "15:00"],
Copy link
Member

@MarcoGorelli MarcoGorelli Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should still be BusinessHour? Sorry, I meant "copy-and-paste but still change the offset", I should've clarified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! I corrected my mistake. The copy-and-paste method can be dangerous sometimes.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - CI failures are unrelated and should be resolved now, I've just made some final suggestions - if you address them and add a commit then CI should hopefully be green

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there 💪

There's some CI failures from the docstring validation script, check https://github.com/pandas-dev/pandas/actions/runs/3707564825/jobs/6284132896

Comment on lines 1514 to 1515
Passing the parameter ``normalize`` equal True, you shift the start
of next business day to midnight.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

equal True -> equal to True

of next business day -> of the next business day

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected my grammar mistakes and updated my PR.

Comment on lines 1658 to 1659
Passing the parameter ``normalize`` equal True, you shift the start
of next business hour to midnight.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

@natmokval
Copy link
Contributor Author

Almost there 💪

There's some CI failures from the docstring validation script, check https://github.com/pandas-dev/pandas/actions/runs/3707564825/jobs/6284132896

Thank you MarcoGorelli, for your help. I checked the errors raised by the docstring validation script in CI and fixed them. It's interesting, why if I run the docstring validation script locally these errors don't occur.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, nice improvement, thanks @natmokval !

@MarcoGorelli MarcoGorelli merged commit 22491dc into pandas-dev:main Dec 17, 2022
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Dec 17, 2022

I checked the errors raised by the docstring validation script in CI and fixed them. It's interesting, why if I run the docstring validation script locally these errors don't occur.

My guess is that you hadn't re-compiled your C extensions (python setup.py build_ext) - you'll need to do that any time you modify .pyx files (as you did here, or as can happen when you fetch and merge upstream/main)

phofl pushed a commit to phofl/pandas that referenced this pull request Dec 17, 2022
…dev#50240)

* DOC: add examples to BusinessHour and BusinessDay I

* DOC: add examples to BusinessHour and BusinessDay II

* DOC: add examples to BusinessHour and BusinessDay III

* DOC: add examples to BusinessHour and BusinessDay IV

* DOC: add examples to BusinessHour and BusinessDay V

* fixup! DOC: add examples to BusinessHour and BusinessDay V
@natmokval
Copy link
Contributor Author

Thank you MarcoGorelli, for your comment. Usually, I run python setup.py build_ext -j 1 , python -m pip install -e . --no-build-isolation --no-use-pep517and smth like python scripts/validate_docstrings.py pandas.offsets.BusinessDay after modifying .pyx files. Otherwise, html won’ be rebuilt.
Probably this time I missed something.

On the other hand, when I tried to reproduce my mistake, I run python setup.py build_ext -j 1 --force, and then
the docstring validation script raised my error. Maybe it would be better to use --force while re-compiling the C extensions.

@MarcoGorelli
Copy link
Member

if you just do python setup.py build_ext, then only the files that have changed since you last compiled will be recompiled, whereas --force recompiles everything - usually the first one's fine (it's what I usually do), but occasionally things go wrong and I have to use --force. hopefully the meson build will make this work a lot better - there's a PR open for that, don't know how far away it is from being ready though

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

Successfully merging this pull request may close these issues.

2 participants