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

Lookup altitude

Open nicomt opened this issue 3 years ago • 1 comments

  • [x] Closes #1516
  • [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:`1516` or this Pull Request with :pull:`1518`. Includes contributor name and/or GitHub username (link with :ghuser:`nicomt`).
  • [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.
  • [ ] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

To make it easier to review, this pull request doesn't replace the default altitude in the Location class or the get_solarposition yet. I will make another pull request once this one is merged. One change to note is _degrees_to_index; I simplified the function. I think a lot of the centering and floating point logic is unnecessary. I even compared the output over several thousands of random integers, and their outputs were identical. Let me know if you rather I change it back.

nicomt avatar Aug 09 '22 13:08 nicomt

FYI, I want to be more transparent about how I generated the altitude dataset. I'm trying to consolidate the generation into one python script so is easy for you guys to run it. I will probably release it as a Github Gist soon.

Also, I want to do a better job quantifying the accuracy of the generated map. I will try to find a good source of ground truth for altitudes and release some error stats.

nicomt avatar Aug 14 '22 17:08 nicomt

Here is the Gist I promised that allows you guys to reproduce the altitude map. https://gist.github.com/nicomt/d2c5f08a8ee3500550be42d8dbee6c1d

While compiling the script, I realized that the lower altitude bound in my first commit was lower than the lowest place on earth (The Death Sea). In this version, I fixed the lower bound to -450m, which removes those bad values and allows me to reduce the step 28m instead of 35m.

Also, I compared the results with a few samples from the Google Elevation API, and here is what I got.

error_diff

It looks like the error is pretty bad in Greenland and Asia but relatively okay elsewhere. Here are the actual results of my test.

Metric Error
Mean Absolute Error 113
Max Error 3067
Mean Squared Error 133069

Let me know if there is something else I can do to help.

nicomt avatar Aug 18 '22 05:08 nicomt

Thanks so much Nicolas, Two very minor nits that aren't blockers and can be improvements in future PR's, I don't want to hold up merging this asap.

  1. in test_lookup_altitude() a fixture might work better instead of looping over test_locations
  2. in location.lookup_altitude() it would be nice if we could adopt pathlib.Path and the / operator for all new code and retire osp.join(). EG:
    import pathlib
    pvlib_path = pathlib.Path(__file__).parent
    filepath = pvlib_path / 'data' / 'Altitude.h5'
    

mikofski avatar Aug 31 '22 04:08 mikofski

@mikofski I will fix those in the next PR. Thanks

nicomt avatar Aug 31 '22 04:08 nicomt