Skip to content

Add cams.get_mcclear function #1172

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

Closed
wants to merge 57 commits into from
Closed

Add cams.get_mcclear function #1172

wants to merge 57 commits into from

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Feb 20, 2021

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

Add a function to retrieve CAMS McClear clear-sky radiation as previously unsuccessfully attempted in #271, #274, #279.

AdamRJensen and others added 30 commits January 15, 2021 15:49
Simplified how the start and end line of the data is determined. Improved documentation, e.g. moved constants outside of function.
Add bsrn file and read_bsrn function
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
The previous values in the COL_SPEC variables were not all correct, leading to incorrect parsing of the data.
File is not complete, as I'm awaiting permission from BSRN to upload test file
Previously the month and year of the file were determined from the filename. This has now been changed such that the month/year is found from within the file's metadata section (second line).
Air temperature was listed as air_temperature in the docstring instead of temp_air.
@AdamRJensen
Copy link
Member Author

@wholmgren I'm back! This time using GitHub Desktop, so hopefully, the code should have Unix end of line characters.

I'm unsure of why the previous commits show up in the this pull request... do you generally delete a forked repository after a successful pull request and refork it for additional pull requests?

I've asked the developing team of the CAMS radiation and McClear services, and have been informed that there is and will not be any demo API email/access. So if tests have to be implemented, we need to register a PVLIB email. Only a registered email address is necessary and no password will ever be public.

The function includes some very convenient features, including the label, integrated, and map_variable arguments, making it very intuitive for Python users. I'd very much like a critical look at the function and feedback on how it can be improved.

@mikofski
Copy link
Member

mikofski commented Feb 21, 2021

Hi @AdamRJensen, thanks for this contribution!

I'm unsure of why the previous commits show up in the this pull request... do you generally delete a forked repository after a successful pull request and refork it for additional pull requests?

Sometimes previous commits show up if your branch is not sync'd with master. Also it looks like this PR is from your "master" branch of your fork rather than a "feature branch" which in my opinion makes it more difficult to manage your pull requests.

  1. create a new feature branch on your laptop from the HEAD of your current working branch, (looks to me like you're working out of the master branch of your fork: AdamRJensen:master)

    path/to/pvlib (master)$ git checkout -b cams_macclear  # or some other descriptive name
    path/to/pvlib (cams_macclear)$
    
  2. now force your master branch to resync with pvlib-python:master so they are mirrors.

    path/to/pvlib (cams_macclear)$ git checkout master
    path/to/pvlib (master)$ git pull -f git@github.com:pvlib/pvlib-python.git master  # overwrites your existing master branch
    path/to/pvlib (master)$ git push origin master  # update your online GitHub pvlib fork with latest master
    
  3. now go back to your feature branch and merge master to make sure you have the latest changes. This should remove those extra commits you're seeing.

    path/to/pvlib (master)$ git checkout cams_macclear
    path/to/pvlib (cams_macclear)$ git merge master  # you may need to resolve a conflict, sometimes in whatsnew
    path/to/pvlib (cams_macclear)$ git push -u origin cams_macclear  # start tracking this branch on GitHub
    
  4. now go online to GitHub and create a new Pull Request from your feature branch "cams_macclear" to pvlib-python:master reference this PR Add cams.get_mcclear function #1172, and then delete this PR. Sorry I don't think there's a way to redirect the source of PR, only the destination. But these steps will clean up this PR so you don't have a bunch of extra commits.

@mikofski
Copy link
Member

@AdamRJensen also I'm wondering if this PR replaces the existing pvlib.iotools.ecmwf_macc package or if they can be combined someway or refactored? Perhaps worth commenting on if you can? thanks!

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

Successfully merging this pull request may close these issues.

2 participants