Add NSRDB GOES v4 to iotools

[williamhobbs]

• Closes Add NSRDB GOES V4 #2326
• 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.

[williamhobbs]

What's the best practice for lines like this one that end up too long? Do I simply add # noqa or # noqa: F401? (I don't know what F401 is for, but I've seen it used here...)

GOES_URL = NSRDB_API_BASE + "/api/nsrdb/v2/solar/nsrdb-GOES-aggregated-v4-0-0-download.csv"

https://github.com/williamhobbs/pvlib-python/blob/4af81a76b4d8bb86b992b5a87ca81dbec6ecb8d5/pvlib/iotools/goes4.py#L15

[mikofski]

You could use parentheses

GOES_URL = NSRDB_API_BASE + ("/api/nsrdb/v2/solar/"
"nsrdb-GOES-aggregated-v4-0-0-download.csv")

or just break the string smaller and add them

API_STUB = "/api/nsrdb/v2/solar/"
ENDPOINT = "nsrdb-GOES-aggregated-v4-0-0-download.csv"
GOES_URL = (NSRDB_API_BASE
    + API_STUB + ENDPOINT)

imo avoid ignoring pep8 warnings or else they become pointless.

[williamhobbs]

Thanks, @mikofski!

[williamhobbs]

I'm not 100% sure that I set up the tests in test_goes4.py correctly. I copied what was done in test_psm3.py and manually downloaded and reformatted as needed the test data csv files.

See test_read_goes4_map_variables() in particular. columns_mapped includes variables like 'Ozone' that are part of a manual download but are not addressed in pvlib.

I think it's correct, but I could be misunderstanding the goals of some of the tests.

[williamhobbs]

Ok, made the switch from "goes4" to "psm4".

I haven't done anything with @kandersolar's suggestion for different get_ functions for different endpoints (e.g., get_nsrdb_goes4_aggregated, get_nsrdb_goes4_conus, and get_nsrdb_goes4_tmy). I'll wait for some consensus on that one.

[kandersolar]

Thanks @williamhobbs for iterating with the tests :) Many of the remaining code coverage issues are for API error handling conditions that we might not be able to easily induce, so I'd say let's ignore those. However, it does look like get_nsrdb_psm4_full_disc isn't currently tested. Can you add another test function for that?

[williamhobbs]

@kandersolar, I think I finally addressed all the tests. Not sure about the failing codecov checks, though [edit: I see you just commented on this...].

One note/question: I manually removed the Alpha and Asymmetry columns from the test psm4 csv files (after removing them from the variable map based on #2378). Was simply deleting those columns from the csv files the right thing to do? I honestly can't remember how I created those files originally.

[williamhobbs]

...it does look like get_nsrdb_psm4_full_disc isn't currently tested. Can you add another test function for that?

@kandersolar, yes, I'll work on that.

[kandersolar]

One note/question: I manually removed the Alpha and Asymmetry columns from the test psm4 csv files (after removing them from the variable map based on #2378). Was simply deleting those columns from the csv files the right thing to do?

It looks like no tests make use of those columns (in the PSM3 tests either). So maybe ideally it would be good to include them, but since there's no special functionality related to those columns that we need to test, I think it's okay to leave them out.

[williamhobbs]

One note/question: I manually removed the Alpha and Asymmetry columns from the test psm4 csv files (after removing them from the variable map based on #2378). Was simply deleting those columns from the csv files the right thing to do?

It looks like no tests make use of those columns (in the PSM3 tests either). So maybe ideally it would be good to include them, but since there's no special functionality related to those columns that we need to test, I think it's okay to leave them out.

I think leaving Alpha and Asymmetry columns in was causing test_read_nsrdb_psm4_map_variables to fail, which is why I removed them.

[kandersolar]

Ok I see. In that case I think the better solution would have been to add the relevant strings to the list of expected keys, rather than remove the columns from the data file.

[williamhobbs]

@kandersolar I think it's ready now.

[kandersolar]

LGTM. Thanks @williamhobbs for this contribution! It's rare to get as many as six functions added in one PR...

I'll open issues for the follow-up items identified in review.

[kandersolar]

I just noticed the default value year='2023' in this function and some others. Is there a good reason for it? I don't see much convenience in having a default year since presumably most users will override it for the year they actually need anyway. And if it's possible to leave the parameter unspecified, it might funnel careless users towards requesting the wrong year unintentionally.

I think we should change it so that year does not have a default value for the calendar year (i.e. non-TMY) functions. @williamhobbs @AdamRJensen any thoughts?

[AdamRJensen]

I think that's a mighty fine idea!

[cwhanse]

Does the NSRDB return an informative error if a year is requested for which there is no data? That might be why a default was included.