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

Hips2fits new service #1734

Merged
merged 11 commits into from
Dec 25, 2020
Merged

Hips2fits new service #1734

merged 11 commits into from
Dec 25, 2020

Conversation

bmatthieu3
Copy link
Contributor

@bmatthieu3 bmatthieu3 commented Jun 4, 2020

Hi @bsipocz @adamginsburg @tboch

I have implemented a new package responsible for querying hips2fits: http://alasky.u-strasbg.fr/hips-image-services/hips2fits
This service take a HiPS name + astropy WCS and return a FITS/JPG/PNG image cutout of the HiPS for that WCS. The list of valid hips name is here: http://aladin.unistra.fr/hips/list
There are two methods:

  • query_with_wcs that accepts a HiPS name + an astropy.wcs
  • query doing the same but accepts different parameters instead of an astropy wcs (the center of projection, fov angle, projection, rotation...)

I added a simple doc containing 2 examples with the corresponding output plot images that can be obtained thanks to hips2fits. Some tests (remote too) have also been added.

Thank you very much for your reviews.

@pep8speaks
Copy link

pep8speaks commented Jun 4, 2020

Hello @bmatthieu3! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-24 14:55:14 UTC

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

This looks really good. Could you fix up the PEP formatting, though? I found a few small corrections that need to be handled.

docs/hips2fits/hips2fits.rst Outdated Show resolved Hide resolved
# The WCS header does not contain the NAXISx keywords
# We can get them from the WCS directly and add them
# to the header
nx, ny = wcs.pixel_shape
Copy link
Contributor

Choose a reason for hiding this comment

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

you may not be able to rely on this being present; if a wcs was created not from a fits Header (i.e., from scratch), it won't have this attribute populated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hips2fits service asks for the size of the result image (width and height), it is mandatory. I changed the code, if the wcs has no pixel shape (pixel_shape None) then I raise an AttributeError saying either to give your wcs the pixel size of the image or to use the other query method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmatthieu3
Copy link
Contributor Author

Hi @adamginsburg,

I have seen a test is failing because it runs tests from the doc api (https://travis-ci.org/github/astropy/astroquery/jobs/695927179) which are remote tests. How to say these doc tests are remote and thus can be skipped ? Thanks

PS: I also added a changelog

@keflavich
Copy link
Contributor

keflavich commented Jun 9, 2020

Add __doctest_skip__ = ['Hips2FitsClass.*'] at the top of the code that has Examples in docstrings (see, e.g., Vizier for examples)

@keflavich
Copy link
Contributor

I don't have permission to push to this branch. It needs a simple rebase to handle changelog conflicts.

@bsipocz
Copy link
Member

bsipocz commented Oct 14, 2020

@keflavich - Does this need another round of review, or mergeable pending resolving the conflict and getting an all green CI?

@bsipocz bsipocz added this to the v0.4.2 milestone Oct 14, 2020
@keflavich
Copy link
Contributor

afaict, this was ready to go once those minor fixes (changelog, rebase, CI pass) were completed.

@bmatthieu3
Copy link
Contributor Author

Hi @keflavich @bsipocz,

I added __doctest_skip__ = ['Hips2FitsClass.*'] to the beginning of the core.py file so that doc tests do not execute (because they are remote).
I do not see the CI tests running.
We are very excited to merge this into the next version of astroquery. Let me know what I should do to make this merged.

@keflavich
Copy link
Contributor

I'm going to merge this on the command line - we need to put the changelog entry into the right section, but otherwise it's good to go.

@keflavich
Copy link
Contributor

The remote tests pop up a blocking imshow - we should avoid that! However, the tests all passed.

@keflavich keflavich merged commit f4c0ad9 into astropy:master Dec 25, 2020
@@ -10,6 +10,7 @@
from astropy import wcs

__all__ = ['hips2fits', 'hips2fitsClass']
__doctest_skip__ = ['hips2fitsClass.*']
Copy link
Member

Choose a reason for hiding this comment

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

@tinumide - when we get here, we should also look for doctest skips like this in the code files, and turn on the remote testing for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, noted

@bsipocz
Copy link
Member

bsipocz commented Jan 5, 2021

@bmatthieu3 @keflavich - Please open a follow-up PR for fixing the imshow issue or skip the test, as we want to turn back on the remote CI testing soonish.

@bsipocz
Copy link
Member

bsipocz commented Jan 12, 2021

Retrospect I think this should have been a new module under cds, as part of our upcoming restructure.

@ManonMarchand ManonMarchand deleted the hips2fits branch October 4, 2023 10:45
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.

5 participants