Adding horizon data retrival methods

[bgpierc]

• [x] Closes #1295, #758
• [x] 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.

In this request, I've added two main functions: one in a new module horizon.py that, given an SRTM DEM, calculates a horizon profile in terms of (azimuth, elevation) manually, and an addition in iotools that fetches the same from pvgis (#1295 ). I also incorporated a few functions from abandoned #758. I've tested that they're compatible, but have not verified the results.

Along with #1295, this gives pvlib 3 potential ways of retrieving the horizon profile. I'm going to be testing them against each other and collected field data like @mikofski suggested in #1295 . I'll add more functionality as I go along but I wanted to check in with @cwhanse and @jsstein with what we have now. I'm currently working on retrieving raw SRTM data automatically, as well.

[cwhanse]

@bgpierc before we start polishing in earnest, could you clear up the stickler items? Makes it much easier to review. My first reactions are 1) that many of the utility functions in horizon.py would be better as private, and 2) several added dependencies should be optional.

[bgpierc]

@cwhanse fixed stickler errors, and added underscores for private functions. How should I specify optional dependencies?

[kandersolar]

Thanks @bgpierc!

How should I specify optional dependencies?

Add an entry for each new dependency here: https://github.com/pvlib/pvlib-python/blob/master/setup.py#L56-L58

And then, rather than importing the optional dependencies at the top of the module like normal (which, when the new functions are eventually added to an __init__.py somewhere, will cause import pvlib to fail if the optional deps are not available), import them inside the functions that use them like this: https://github.com/pvlib/pvlib-python/blob/master/pvlib/bifacial.py#L95

[bgpierc]

Perfect, I added new dependencies to the optional section. I also changed the horizon_map function to accept an array for the DEM rather then a path, as there's a few different file types (GeoTiff, ascii, .hgt) that vary depending on where you get the data from. I'm currently working on an automated download function for DEMs, so I was meaning to change that anyway.

[bgpierc]

Thanks @cwhanse , went through and made the changes. I did have a question about attributing the code from #758 ; is there a permalink to that pull request if it's closed? Is there a better way to reference that?

I moved those functions to shading.py, I agree they're better there. I have kept the functions in horizon.py public, as I think they're either needed or useful to expose to the end user.

I also edited the angle calculation method: I realized what I had was incorrect as a) it didn't include a y-coordinate for the "adjacent" distance (this wasn't caught because my test cases were all on roughly the same y-axis plane) and b) forgot to convert from pixels to meters.

This didn't effect the overall shape of the output elevation, just the size of the elevation angle. It seems to match up reasonably? closely to pvgis & the method provided in #1295 (see figure); maybe due to using different versions/interpolations of the SRTM?

[kandersolar]

I did have a question about attributing the code from https://github.com/pvlib/pvlib-python/pull/758 ; is there a permalink to that pull request if it's closed? Is there a better way to reference that?

Pull request links are valid even after closing, but in the source code we typically just refer to GH #758 or similar rather than the full url.

I think the ideal solution to retain commit authorship would have been to preserve the original commits themselves using git rather than copy/pasting their content and making new commits. Then the original author would show up in in this PR's commit history. Might be too late for that now unless you basically start over.

Regardless, we should be sure to include the original author in the eventual "what's new" entry!

[adriesse]

This didn't effect the overall shape of the output elevation, just the size of the elevation angle. It seems to match up reasonably? closely to pvgis & the method provided in #1295 (see figure); maybe due to using different versions/interpolations of the SRTM?

If the green line in the diagram above is supposed to be close to the others, then I would say it does not match up reasonably. A possible reason for this could be the horizon limit. The default 500m was used it is likely to be too small. 10 km might be a better default, but even that will be too small in many locations.

[cwhanse]

I think the ideal solution to retain commit authorship would have been to preserve the original commits themselves using git rather than copy/pasting their content and making new commits.

I agree in principle, but in this case #758 contains a lot of other code that isn't going forward in this PR.

[bgpierc]

The default 500m was used it is likely to be too small. 10 km might be a better default, but even that will be too small in many locations.

I'm working on adding larger ranges; this involves stitching DEM tiles together as an individual tile may be too small if the point of interest in the a corner or edge of the DEM.

Additionally, @jsstein suggested that instead of choosing the point of maximum elevation (meters), then calculating elevation (degrees), calculate the angle first and pick the maximum of that, which, in hindsight, makes more sense. If we apply that, we get the attached figure:

This appears to miss a geographical feature from 100-200 degrees azimuth, which I think is because of the aforementioned outmapping problem.

[bgpierc]

Ok, I added a function to download the DEM tiles. I have it accept a latitude and longitude and download which ever DEM tile that contains it. I made a new module in iotools because I wasn't sure where else to put it since it's introducing a new data source, is that good?

I choose to use CGIAR as a source instead of USGS EarthExplorer because CGIAR doesn't require a login and I didn't think we wanted to have to deal with authentication.

[mikofski]

@bgpierc sorry if you already answered this, but I do use USGS Earth Explorer - so if I have already downloaded a DEM file or four, can I still use horizon.py to calculate the horizon with the DEM file on my local disk? Or does this only retrieve data from CGIAR? Thanks!

[bgpierc]

@mikofski yep, as long as the elevation argument to horizon_map is a 2D array with values being elevation in meters, it'll work. You'll have to be careful to select the correct resolution, usually 30m or 90m. I'm also looking into differences between various SRTM releases/versions as well, like void-filled vs non-void filled, different interpolation methods, etc.

[bgpierc]

Sorry, meant to get back to this sooner. Publication on the DEM method to calculate the horizon map manually is coming along slower then expected, mostly due to difficulties getting validation data, so I took that out for now.

There are now three things added in this commit:

1. horizon.py , which only contains the direct and diffuse adjustments from #758 for now
2. a method get_pvgis_horizon in iotools which uses requests to fetch from pvgis's API
3. iotools/cgiar.py, which adds a method to programmatically download a DEM for a given lat/lon

I also put in a test for pvgis, hopefully I did it right. I think it makes sense to start with this and then add the manual horizon calculation in later to avoid adding too much at once.

[kandersolar]

Thanks @bgpierc, is this PR ready for a close review? All the new code will need tests to verify that they execute without error, work with all the advertised types, etc.

Also it seems like the new functions in shading.py and horizon.py are duplicates of each other, unless I'm missing something.

[bgpierc]

Hi @kanderso-nrel oh, I forgot that those are in shading.py already. That probably makes more sense; I can add a new module topography.py or something like that later on for all the code with the DEMs since you can do other things with them that are not related to the horizon.

I added a test for the pvgis function, but I am unsure how to do one for iotools/cgiar.py. The DEM files are rather large (~70MB) so it may not be ideal to distribute one for testing.

[kandersolar]

I added a test for the pvgis function, but I am unsure how to do one for iotools/cgiar.py. The DEM files are rather large (~70MB) so it may not be ideal to distribute one for testing.

I agree a 70MB file is quite large for pvlib. I'm not familiar with DEMs but it looks like it's more or less a 2d array of elevation data. Instead of testing against the full grid, would it be reasonable to determine the expected elevations for just a few locations and hardcode those values in the tests?

[jsstein]

Is there a way to have the test include the download the file as part of the test?

From: Kevin Anderson @.> Reply-To: pvlib/pvlib-python @.> Date: Monday, October 31, 2022 at 12:06 PM To: pvlib/pvlib-python @.> Cc: Joshua Stein @.>, Mention @.***> Subject: [EXTERNAL] Re: [pvlib/pvlib-python] Adding horizon data retrieval methods (PR #1395)

I added a test for the pvgis function, but I am unsure how to do one for iotools/cgiar.py. The DEM files are rather large (~70MB) so it may not be ideal to distribute one for testing.

I agree a 70MB file is quite large for pvlib. I'm not familiar with DEMs but it looks like it's more or less a 2d array of elevation data. Instead of testing against the full grid, would it be reasonable to determine the expected elevations for just a few locations and hardcode those values in the tests?

— Reply to this email directly, view it on GitHubhttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpvlib%2Fpvlib-python%2Fpull%2F1395%23issuecomment-1297471779&data=05%7C01%7Cjsstein%40sandia.gov%7C173ab1500e214aa7d95208dabb6a9601%7C7ccb5a20a303498cb0c129007381b574%7C1%7C0%7C638028363714631122%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=YoYeN6Y01nIBTU2VfXGCFVFMQjuAG5JaNgOzOkS8KFg%3D&reserved=0, or unsubscribehttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABJES5BQGPW2JWZEBKP3KOTWGADAPANCNFSM5M4AUZCQ&data=05%7C01%7Cjsstein%40sandia.gov%7C173ab1500e214aa7d95208dabb6a9601%7C7ccb5a20a303498cb0c129007381b574%7C1%7C0%7C638028363714631122%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6s2UlGURek7g%2BEk98iimZmIAkecKMCh7zkQB%2B1hFGLM%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

[bgpierc]

@kanderso-nrel @jsstein OK, I added a test that downloads the DEM and then verifies that three hardcoded points are the same elevation. I had the function place the downloaded file in the ./pvlib-python/pvlib/data directory.

The function to download the data raises an HTTP error if it fails to fetch as well, does that need to be integrated into pytest with a with pytest.raises(HTTPError): context block? sorry, I'm not very familiar with pytest.

[bgpierc]

@kandersolar @jsstein The horizon functions in this PR have been updated and appear to have no conflicts. I think I did the tests look right, but they could use a second look.

Anyway, to recap, this PR primarily contains a function to download the horizon for a given lat/lon from PVGIS, as well as a DEM downloading function. Do we want to merge this?

[cwhanse]

@bgpierc can you update the PR checklist?

[bgpierc]

@cwhanse done, it seems like docs/sphinx/source/api.rst no longer exists, is there a replacement or somewhere else to modify?

[cwhanse]

@bgpierc https://github.com/pvlib/pvlib-python/blob/main/docs/sphinx/source/reference/iotools.rst

[bgpierc]

@cwhanse done, thanks! The checklist should be up to date, then.

[kandersolar]

@bgpierc I'm not sure what's going on with the three windows test failures, but it seems like some transient PyPI issue that isn't relevant to this PR, so OK to ignore

[bgpierc]

@kandersolar ok, removed all the DEM and shading stuff, now only the pvgis horizon method is added here (diff). I think we should be good to go now.

[AdamRJensen]

Since there are only two columns, I would vote for setting the azimuth as the index.

[bgpierc]

@AdamRJensen Setting horizon_azimuth to the index makes sense to me, done

[AdamRJensen]

Given that this is an iotools function, it could be considered following the convention of returning a tuple of (data, meta). For this function, the meta object would just be an empty dictionary.

If we can imagine that other horizon fetching functions could return metadata then I would vote for doing this.

[kandersolar]

For this function, the meta object would just be an empty dictionary.

Well, the PVGIS API does actually return metadata, we just aren't doing anything with it right now. I'm in favor of having this function follow the data, metadata convention and passing through the metadata returned by the API. Good idea!

[bgpierc]

Ok, I had it return the meta data from PVGIS as well. I'm not sure if we want to make the function return a Series rather the a DataFrame as the other pvgis functions all return a DataFrame. Also, I think having the horizon_azimuth be explicit as the index column is helpful for clarity.