Skip to content

Deprecate parse_psm3 and parse_cams #2458

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 7 commits into from
May 23, 2025

Conversation

kandersolar
Copy link
Member

  • Progress towards Consider removing the parse_ iotools functions #2444
  • I am familiar with the contributing guidelines
  • Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • 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.

To keep reviews manageable, I thought it better to not deprecate all the parse_ functions in one PR. This one does just PSM3 and CAMS, which are both straightforward.

@kandersolar kandersolar added this to the v0.12.1 milestone May 20, 2025
@kandersolar kandersolar added api remote-data triggers --remote-data pytests labels May 20, 2025
@kandersolar kandersolar requested a review from AdamRJensen May 20, 2025 14:18
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

LGTM!

I really think this is a good move, and will reduce maintenance burden in the future.

@AdamRJensen AdamRJensen requested review from RDaxini and echedey-ls May 23, 2025 00:01
Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

Absolute LGTM. Two minor nitpicks down below, feel free to ignore one of them (at most!).
Just as a side note, I prefer setting the versions at which they will be removed from the start, but it's not a strong opinion.

"""
Parse an NSRDB PSM3 weather file (formatted as SAM CSV). The NSRDB
Read an NSRDB PSM3 weather file (formatted as SAM CSV). The NSRDB
is described in [1]_ and the SAM CSV format is described in [2]_.

.. versionchanged:: 0.9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. versionchanged:: 0.9.0

May be time to clean-up this admonition

Copy link
Member Author

@kandersolar kandersolar May 23, 2025

Choose a reason for hiding this comment

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

+1, but I suggest we do it as part of a wider survey of old versionchangeds that could be removed

@kandersolar
Copy link
Member Author

Just as a side note, I prefer setting the versions at which they will be removed from the start, but it's not a strong opinion.

I know. The argument in favor of leaving it unspecified is that it gives us flexibility to make the *.0 releases sooner than we expected, and I think the length of time is more important than the version numbers. (I know you know this, I am just explaining so that you don't think there is some better reason.)

@kandersolar kandersolar merged commit f33be83 into pvlib:main May 23, 2025
30 checks passed
@kandersolar kandersolar deleted the deprecate-parse branch May 23, 2025 01:54
@echedey-ls
Copy link
Contributor

Just as a side note, I prefer setting the versions at which they will be removed from the start, but it's not a strong opinion.

I know. The argument in favor of leaving it unspecified is that it gives us flexibility to make the *.0 releases sooner than we expected, and I think the length of time is more important than the version numbers. (I know you know this, I am just explaining so that you don't think there is some better reason.)

Explanations are always welcome. Thanks @kandersolar .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants