Skip to content

Remove ecmwf_macc functions #1654

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 9 commits into from
Jun 8, 2023

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Feb 2, 2023

  • Closes ECMWF MACC will be retired #1691
  • 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.

This PR removes the get_ecmwf_macc and read_ecmwf_macc functions. The reasoning is that the MACC dataset is now outdated (covers the period 2003-2012) and the functions are hardly ever used thus it is not worth maintaining these functions any longer.

For more recent reanalysis data, users are encouraged to check out ECMWF ERA5, CAMS, or NASA MERRA-2 as a starting point.

See this link for a comparison between MACC and CAMS.

@AdamRJensen AdamRJensen added the io label Feb 2, 2023
@AdamRJensen
Copy link
Member Author

@kanderso-nrel I guess you will insist we deprecate these functions instead of bluntly removing them? 😆

@AdamRJensen AdamRJensen added this to the 0.9.5 milestone Feb 2, 2023
@cwhanse
Copy link
Member

cwhanse commented Feb 2, 2023

@kanderso-nrel I guess you will insist we deprecate these functions instead of bluntly removing them? 😆

if @kanderso-nrel doesn't, I will.

@kandersolar
Copy link
Member

insist we deprecate these functions instead of bluntly removing them?

Perhaps not insist, but strongly endorse :) It costs little to maintain so I see little reason to not do a typical deprecation cycle.

@mikofski
Copy link
Member

mikofski commented Feb 2, 2023

I should stress that:

the MACC dataset is now outdated (covers the period 2003-2012)

is not IMHO a valid reason for deprecating this function. EG: one could say that TMY2 is outdated, but I predict we will never deprecate read_TMY2.

However, this reason:

and the functions are hardly ever used thus it is not worth maintaining these functions any longer.

is IMHO a good reason to do away with something that costs us to maintain, but does not pay any dividends if no one is using it

@adriesse
Copy link
Member

adriesse commented Feb 2, 2023

does not pay any dividends

Let me check my bank statement whether this is really true...

@kandersolar kandersolar modified the milestones: 0.9.5, Someday Feb 20, 2023
@mikofski
Copy link
Member

Should we consider archiving the MACC data set somehow somewhere?

@kandersolar kandersolar modified the milestones: Someday, 0.9.6 May 4, 2023
@AdamRJensen
Copy link
Member Author

I think it's fair to merge this PR soon as the dataset is removed on June 1st and our next release will surely be after that.

@AdamRJensen AdamRJensen added the remote-data triggers --remote-data pytests label May 11, 2023
@AdamRJensen AdamRJensen modified the milestones: 0.9.6, 0.10.0 May 16, 2023
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

LGTM, pending the actual dataset removal on the ECMWF side.

@kandersolar
Copy link
Member

@AdamRJensen points out that the MACC dataset decommissioning is complete, so I think the time has come to merge this PR.

@kandersolar kandersolar merged commit 5f6aef6 into pvlib:main Jun 8, 2023
@kandersolar kandersolar mentioned this pull request Jun 8, 2023
9 tasks
@kandersolar kandersolar mentioned this pull request Jun 29, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECMWF MACC will be retired
5 participants