Feat xxx extract epic

[lvalerom]

This a new feature in the XMM Newton module that allows to extract the EPIC images for different bands and instruments from a given TAR file.

[pep8speaks]

Hello @lvalerom! 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-09-16 05:38:28 UTC

[codecov]

Codecov Report

Merging #1759 into master will decrease coverage by 0.11%.
The diff coverage is 94.36%.

@@            Coverage Diff             @@
##           master    #1759      +/-   ##
==========================================
- Coverage   63.89%   63.77%   -0.12%     
==========================================
  Files         200      199       -1     
  Lines       15736    15614     -122     
==========================================
- Hits        10054     9958      -96     
+ Misses       5682     5656      -26     

Impacted Files	Coverage Δ
astroquery/esa/xmm_newton/core.py	73.97% <94.36%> (+19.30%)	⬆️
astroquery/splatalogue/build_species_table.py	38.46% <0.00%> (-2.28%)	⬇️
astroquery/utils/commons.py	76.77% <0.00%> (-0.23%)	⬇️
astroquery/alma/utils.py	29.13% <0.00%> (-0.09%)	⬇️
astroquery/sdss/core.py	86.33% <0.00%> (ø)
astroquery/alma/__init__.py	100.00% <0.00%> (ø)
astroquery/splatalogue/__init__.py	100.00% <0.00%> (ø)
astroquery/alma/tapsql.py
astroquery/alma/core.py	35.42% <0.00%> (+1.31%)	⬆️
astroquery/gemini/core.py	68.68% <0.00%> (+3.46%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56b213a...db5fb1e. Read the comment docs.

[lvalerom]

Good morning,

We encountered a minor problem with the specification of this new feature. We have already prepared a fix for it. We were wondering if it would be better to push the changes to this pull request or wait until this one is closed and open a new one later.

Please advise.

Regards

[keflavich]

Add it to this PR.

[keflavich]

The docstring needs a bit of cleanup to format correctly.

Also, if there is a real error, like "file not found", that should be raised as an exception, not logged as a warning.

[keflavich]

Also, acronyms need to be defined. In particular, EPIC is not defined anywhere.

This feature should be included in the narrative docs too (docs/esa/xmm_newton.rst)

[lvalerom]

Good morning,

Thank your for your feedback, I believe I've fixed everything that was suggested.

Please let me know if there are any additional changes you advise.

Thank you
Luis

[keflavich]

It looks like this new utility is not doing any sort of remote query, it is just extracting specific files from a local tarfile. Is that correct? If so, I suggest we refactor this to match other astroquery modules: let's move this new feature to utils.py and have it work as a standalone. e.g., the documentation would say:

tarfilename = XMMNewton.download_data(...)
epic_images = xmm_newton.utils.extract_epic_images(tarfilename)

The prefix get_ is reserved for remote queries.

@bsipocz @ceb8 Please comment on this suggested change - are you onboard with the pattern of using utils.py for non-remote operations? Or should this remain a method on the XMMNewton class? Which is more intuitive?

[keflavich]

No, if an additional query needs to be done, keep that in the main module, probably. Could you describe a little more what the intended use case is for that? Why would you need an additional file when un-taring a local file?

[lvalerom]

In that particular case, the PPS tar file does not include the Response Matrix File (RMF) needed for the analysis of the spectral products. I'm not sure as to the reason why they did not include this file there but the only way to retrieve the appropriate RMF is to download it from an FTP server.

[lvalerom]

Good morning @keflavich

Are there any updates regarding the suggestion of moving this feature to utils.py? Please let me know if you need anything else from my side.

Thank you

[keflavich]

I don't think this needs to be moved to utils after all. It's good to go from my perspective, I just added a minor typo correction & some styling improvements.

@ceb8, @bsipocz, could either of you do a final check & merge?

[bsipocz]

@keflavich - I do a final check and rebase, there are plenty of unrelated commits present.

[bsipocz]

I've cleaned up the commit history and rebased, once travis passed this can be merged, after adding a changelog.

[bsipocz]

I've added the changelog to get it merged

[bsipocz]

Thanks @lvalerom!