Skip to content

Implement MoonLocation.of_site#50

Merged
aelanman merged 1 commit into
aelanman:mainfrom
lpsinger:of_site
May 12, 2026
Merged

Implement MoonLocation.of_site#50
aelanman merged 1 commit into
aelanman:mainfrom
lpsinger:of_site

Conversation

@lpsinger

@lpsinger lpsinger commented May 5, 2026

Copy link
Copy Markdown
Contributor

Look up a surface feature in the USGS Gazetteer of Planetary Nomenclature (https://planetarynames.wr.usgs.gov/Page/MOON/target).

  • This data source provides the longitude and latitude, but not the height, of the approximate centers of craters and other features.
  • Locations are referenced to the IAU2000 selenoid.
  • This function downloads a KML file that is 11M in size the first time that you call it in a given Python session.

Fixes #10.

Note: merge #49 first.

@aelanman aelanman mentioned this pull request May 11, 2026
@lpsinger lpsinger force-pushed the of_site branch 2 times, most recently from b5c5bee to f353bbe Compare May 11, 2026 21:29
@lpsinger

Copy link
Copy Markdown
Contributor Author

This is ready for review. An important first question is if this is a useful definition of "of_site" or if height from the datum is essential.

@aelanman aelanman left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lpsinger ; This looks like a very useful addition!

A couple notes:

  1. Does the site it's querying provide heights above datum for each selenodetic position? I assume not, since you left it an open question. I'm happy to skip that for now if it's unavailable.
  2. You should add at least one unit test to cover the new feature, and possibly update the github actions to cache the klm file per-OS. I can help with that if you need.

@lpsinger

Copy link
Copy Markdown
Contributor Author
  1. Does the site its querying provide heights above datum for each selenodetic position? I assume not, since you left it an open question. I'm happy to skip that for now if it's unavailable.

It does not.

  1. You should add at least one unit test to cover the new feature, and possibly update the github actions to cache the klm file per-OS. I can help with that if you need.

I added a doctest.

It is not cached. It was unclear how frequently this file is updated, so I didn't cache it.

@aelanman

Copy link
Copy Markdown
Owner

Thanks for adding the test. I just meant caching for the test suite (like how the kernel files are cached). This is done to reduce the download requests from 10 to 2 each time the testsuite workflow is triggered.

Look up a surface feature in the USGS Gazetteer of Planetary
Nomenclature (https://planetarynames.wr.usgs.gov/Page/MOON/target).

- This data source provides the longitude and latitude, but not the
  height, of the approximate centers of craters and other features.
- Locations are referenced to the IAU2000 selenoid.
- This function downloads a KML file that is 11M in size the first
  time that you call it in a given Python session.

Fixes aelanman#10.
@lpsinger

Copy link
Copy Markdown
Contributor Author

#55 will fix the unit tests.

@aelanman aelanman left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge this now if you think it's ready. I don't want to drop support for older python versions just yet, since one of the main users of this package still supports older versions. Also, it looks like the namespace failure on path.glob is not just a result of python versioning. I'll see if I can fix the tests

@aelanman aelanman merged commit aee3496 into aelanman:main May 12, 2026
10 of 14 checks passed
@lpsinger lpsinger deleted the of_site branch May 13, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add known lunar sites

2 participants