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

Bugfix for empty scheduling block list #298

Merged
merged 1 commit into from
Apr 26, 2017

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Apr 14, 2017

Hoping this is the last fix blocking the update for astropy 1.3 compatibility, which affects #296, #292, #282.

@bmorris3
Copy link
Contributor Author

bmorris3 commented Apr 14, 2017

Hey @eteq – I have a question for you about the implementation of sites within EarthLocation in astropy. It looks like when setup.py test is run on astroplan without --remote-data, astropy is installed without remote data, and therefore the only site in the registry is Greenwich. Previously, astroplan had monkey-patched APO, Subaru, and maybe other sites into the Observer.at_site method so that we could still run some doctests even without --remote-data.

It looks like that's now out of date, and this issue is the last blocker that we need to fix in order to release a much needed bugfix update for astroplan.

Do you have thoughts on how best to approach this problem? Would you accept a pull request to sites.json that includes the site information for the sites that astroplan tests on, or should we re-work those astroplan tests away from real observatories (like this one that uses Subaru, for example)? Or we could re-write the monkeypatch for astroplan (this time onto EarthLocation) just for those sites that we use in the astroplan tests.

Perhaps there's already a patch being done here, and the patch is failing for reasons I don't understand (failure error here)?

cc @bsipocz – we're so close to being ready for that bugfix release.

@bsipocz
Copy link
Member

bsipocz commented Apr 14, 2017

@bmorris3 - this seems to be very similar to the issue we have currently in astropy, e.g. https://travis-ci.org/astropy/astropy/jobs/221883232#L2337

I suggest to use a local monkeypatch rather than to add a few random obs to astropy's default. The latter will raise questions about the selection of obs getting into that file while others are left out and we've already been down that road before.

@bmorris3 bmorris3 merged commit 6a109cf into astropy:master Apr 26, 2017
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.

2 participants