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

sam iotools #1371 #1556

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

sam iotools #1371 #1556

wants to merge 20 commits into from

Conversation

shirubana
Copy link

@shirubana shirubana commented Sep 22, 2022

adds a sam.py, and pytest

  • I think Closes Add iotools function to write SAM-formatted CSV files #1371
  • 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.

@cwhanse cwhanse added this to the 0.9.4 milestone Sep 22, 2022
@cwhanse
Copy link
Member

cwhanse commented Sep 22, 2022

@shirubana when sticker turns green we'll approve and run the tests.

@kandersolar kandersolar added the remote-data triggers --remote-data pytests label Sep 22, 2022
@kandersolar
Copy link
Member

@williamhobbs, since you opened #1371, would you be interested in taking a look at this PR from a user's perspective?

@williamhobbs
Copy link
Contributor

@williamhobbs, since you opened #1371, would you be interested in taking a look at this PR from a user's perspective?

Sure! I just ran a test with some data I previously grabbed with pvlib.iotools.read_surfrad. Here are a few comments:

  • The metadata dictionary returned by pvlib.iotools.read_surfrad stores the time zone as "tz": "UTC". Two issues come up from that:
    • sam.py is looking for "TZ" instead of "tz". Not sure if this matters, since "tz" doesn't seem to be standardized across iotools, but it might help a bit to be consistent.
    • NREL SAM needs the time zone as a number that is UTC offset, e.g., 0 for UTC or -6 for MDT. It would be nice if sam.py could return an error or warning when passing a time zone string, or even better, convert time zones that might get created by other functions in iotools into the number fomat SAM is looking for.
  • When filling in the year so that it starts Jan-1 at 12 AM and ends with the last interval of the calendar year, I typically "roll" the data if it starts mid-year (e.g., if I have data from March 2018 through Feb 2019, move Jan and Feb 2019 to the beginning of the file). In my test case, I pulled data for a full calendar year in UTC (starting at 2019-01-01 00:00 UTC and ending 2019-12-31 23:59 UCT), but after running through sam.saveSAM_WeatherFile(), I ended up with a two-year weather file since 2019-01-01 00:00 UCT is 2018-12-31 18:00 CST and it filled in zeros for the rest of 2018. I'm not sure the best way to handle this, but I think the current approach could be confusing.
  • Naming the function write_sam() or similar might better match existing naming conventions, but I'm also not familiar with pvlib function name guidelines.
  • sam.saveSAM_WeatherFile() requires a source value in the metadata file, but that isn't produced by read_surfrad and some of the other iotools functions. Would it makes sense to have this function default to something, e.g., "pvlib export", if the user doesn't provide an input value?

got it wrong, it should be left labeled. Added other values of use for SAM for wind simulations
The function description states that it reads SAM files, but the SAM files does not need a 'Local Time Zone' in the metadata, as well as the Minute column is optional.

When Minutes are included in the SAM file, it considers the sun position needs to be calculated at that specific point, instead of calculating it for left labeled data (timestamp 10 AM is data left averaged from 10 AM to 11 AM, and SAM places Sun position at 10:30 AM).
-metadata now uses and expects 'tz' (non caps).
-If tz data is not numberic, it throws an error.
-function renamed to write_sam()
-source value set to 'pvlib export' if not defined.
@shirubana
Copy link
Author

@williamhobbs do you have an example of your rolling method?

I addressed everything else except that and seems veyr important to do so, and also the timezone string to number function, is a nice add on in general but I probably won't get around to that specifically for this PR.

shirubana and others added 3 commits October 14, 2022 16:18
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
addresses data looping when data goes from example June one year to May the next one.
@shirubana
Copy link
Author

@williamhobbs I added a rolling method. Whenever you get an opporutnity can you re-try your surfrad data?

Thanks!

@williamhobbs
Copy link
Contributor

@shirubana The rolling seems to work based on a quick test.

I'm getting the time zone in the output file as "UTC" instead of "0". I think users would expect the time zone in the metadata returned from read_surfrad to work, or to get an error/warning.

I've also just realized that it would be helpful to be able to split out hourly averaging from the other standardSAM behaviors (skipping leap day and making the file start with Jan 1 12:00 AM), since SAM does not require hour intervals but does require the other items. I would suggest either:

  1. removing the resampling (_averageSAMStyle) or
  2. creating an additional input that is either a boolean for hourly averaging or something that lets you pass an averaging interval in minutes.

@kandersolar kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@kandersolar kandersolar modified the milestones: 0.9.4, 0.9.5 Dec 14, 2022
@kandersolar kandersolar modified the milestones: 0.9.5, 0.9.6 Mar 18, 2023
@kandersolar kandersolar modified the milestones: 0.9.6, 0.10.0, Someday May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement io remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add iotools function to write SAM-formatted CSV files
5 participants