pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

sam iotools #1371

Open shirubana opened this issue 2 years ago • 6 comments

adds a sam.py, and pytest

  • [X] I think Closes #1371
  • [X] I am familiar with the contributing guidelines
  • [X] Tests added
  • [x] Updates entries in docs/sphinx/source/reference for API changes.
  • [x] 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`).
  • [X] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [X] Pull request is nearly complete and ready for detailed review.
  • [x] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

shirubana avatar Sep 22 '22 18:09 shirubana

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

cwhanse avatar Sep 22 '22 18:09 cwhanse

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

kandersolar avatar Sep 23 '22 17:09 kandersolar

@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?

williamhobbs avatar Sep 23 '22 22:09 williamhobbs

@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 avatar Oct 13 '22 22:10 shirubana

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

Thanks!

shirubana avatar Oct 14 '22 23:10 shirubana

@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.

williamhobbs avatar Oct 15 '22 00:10 williamhobbs