Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing the astroplan sites module (which migrated to astropy) #168

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

bmorris3
Copy link
Contributor

The astroplan sites module was added in #45, and then ported to astropy in astropy/astropy#4042. Since then, we've been meaning to remove the now redundant sites machinery from astroplan. In this PR, I removed the sites module and associated tests.

In addition, I had to create a new monkey patch. Many of the tests for the astroplan.Observer class used the class method astroplan.Observer.at_site to efficiently resolve coordinates for an observatory with the original sites module. Since the original sites module had a local copy of the site database stored offline, the tests written with astroplan.Observer.at_site would function identically in tests with or without the --remote-data flag. Now that we're using EarthLocation.of_site under the hood which always accesses the internet, I had to monkey-patch the EarthLocation.of_site method if the --remote-data option is not used, so that the tests can still resolve coordinates for the observatories in the tests without internet access.

This closes #121 and #40.

cc @cdeil, my monkey-patch tutor.

@bmorris3
Copy link
Contributor Author

These tests will fail until we update the required astropy version to >1.1 (I think that's the version with EarthLocation.of_site). Since in #167 I raise the requirements to astropy>=1.2, we might as well wait to retest/merge this PR until #167 is merged.

@cdeil
Copy link
Member

cdeil commented Jun 13, 2016

I think it's OK if astroplan 0.2 will require astropy 1.2.

@bsipocz
Copy link
Member

bsipocz commented Jun 13, 2016

@bmorris3 - this also closes #119

@ejeschke
Copy link
Contributor

It would be nice if EarthLocation.of_site in astropy would not have to access the internet every time it is called. Is it not written to use a cache between process invocations, @bmorris3 ?

@bmorris3
Copy link
Contributor Author

@ejeschke – the method caches the observatory database on first access, then accesses the cache each time unless it's forced to re-download, see source here. The monkey patch was needed because the tests would have to access the remote database once in order to download the database.

@bmorris3
Copy link
Contributor Author

bmorris3 commented Jul 27, 2016

With the IERS errors fixed, this is ready for merge. This closes #119, #121 and #40.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Observer.at_site to use astropy site machinery
4 participants