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

Replace freq '1M' with '1MS' #2266

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Oct 16, 2024

  • I am familiar with the contributing guidelines
  • Tests added
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

The freq='1M' is used in a couple of places in the pvlib iotools functions, which results in the following future warning:

FutureWarning: 'M' is deprecated and will be removed in a future version, please use 'ME' instead.
months = pd.date_range(

The frequency 'M' is being phased out in favor of 'ME' and 'MS' which correspond to month end and month start. 'M' is equivalent to 'ME'. However, 'MS' is the only one supported in our minimum Pandas version, whereas 'ME' was first introduced in a later version. For the iotools functions, I think 'MS' is conceptually more correct.

Minimum changes to the lookup linked turbidity functions were made to make tests pass. I preferred making these modifications over calculating new test values.

@AdamRJensen AdamRJensen added this to the v0.11.2 milestone Oct 16, 2024
@AdamRJensen AdamRJensen changed the title Replace freq '1M' with '1ME' Replace freq '1M' with '1Ms' Oct 17, 2024
@AdamRJensen AdamRJensen added the remote-data triggers --remote-data pytests label Oct 17, 2024
@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels Oct 17, 2024
@AdamRJensen
Copy link
Member Author

AdamRJensen commented Oct 17, 2024

I ended up removing the label parameter in the cams iotools function. Note, no other iotools function supports this and it turned out to be a maintenance burden that I don't want to support in the future. This means that this PR should be made in a .0 release (for this reason I'm holding off adding a whatsnew entry).

Note, test failures are unrelated.

@AdamRJensen AdamRJensen changed the title Replace freq '1M' with '1Ms' Replace freq '1M' with '1MS' Oct 17, 2024
@AdamRJensen AdamRJensen marked this pull request as ready for review October 17, 2024 16:34
@kandersolar kandersolar modified the milestones: v0.11.2, v0.12.0 Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants