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

Add map_variables parameter to read_tmy3 function #1623

Merged
merged 50 commits into from
May 24, 2023

Conversation

ooprathamm
Copy link
Contributor

@ooprathamm ooprathamm commented Dec 21, 2022

  • Closes Add variable mapping of read_tmy3 #1517
  • 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.

@AdamRJensen
Copy link
Member

Hi @ooprathamm

Thank you very much for tackling this issue 😀

There's a couple of formatting issues (stickler). If you can solve those, then I'll go ahead and give a detailed review. If you have any issues feel free to post here and we'll get it sorted!

Also, if you update the description with the issue number (closes #xxxx), then GitHub will automatically link the two.

@AdamRJensen
Copy link
Member

@ooprathamm Mapping of variables using the _recolumn function is not in line with standard pvlib names. Thus I suggest you create a dictionary mapping the TMY3 names to standard pvlib names. An incomplete list of standard names can be found here. You can check out some of the other iotools to get an idea of how this is done, e.g., PSM3, PVGIS, and CAMS (SODAPRO).

@AdamRJensen AdamRJensen added this to the 0.9.5 milestone Dec 30, 2022
@ooprathamm ooprathamm changed the title Added map_variables param in read_tmy3 func #1517 Added map_variables param in read_tmy3 func Jan 1, 2023
@AdamRJensen
Copy link
Member

@ooprathamm are you still working on this issue? Again, don't hesitate to reach out for help if you are stuck.

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.

It's looking very good so far, well done!

Besides the few comments I've added, there needs to be added one or two tests that checks if the variable mapping does what it is supposed to. The tests for the TMY3 functions are located here.

You can check out this test from the surfrad functions for inspiration.

It would also be ideal to have one test case that checks if the warning is raised properly, see this test case for how to do that.

ooprathamm and others added 7 commits January 22, 2023 18:12
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
@AdamRJensen AdamRJensen added the remote-data triggers --remote-data pytests label Jan 23, 2023
@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels May 17, 2023
@AdamRJensen AdamRJensen requested a review from kandersolar May 17, 2023 05:35
AdamRJensen and others added 5 commits May 22, 2023 17:49
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
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.

I think I like how read_tmy3 looks now :)

I think this PR will be ready after cleaning up the deprecation warnings emitted elsewhere in pvlib: some tests in test_location.py and test_soiling.py (https://github.com/pvlib/pvlib-python/actions/runs/5047873887/jobs/9055325947?pr=1623#step:9:66), plus several pages in the user guide and gallery. Some notebooks too, but I suggest leaving those to continue collecting dust and falling out of date. $ git grep read_tmy3 is helpful in tracking down all the places read_tmy3 gets used.

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 stickler stuff

@kandersolar kandersolar merged commit 50e1998 into pvlib:main May 24, 2023
@kandersolar
Copy link
Member

Thanks @ooprathamm and @AdamRJensen! What a big job this turned out to be.

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

Successfully merging this pull request may close these issues.

Add variable mapping of read_tmy3
4 participants