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

Improve tests for HESS data #473

Merged
merged 8 commits into from Mar 16, 2016

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Feb 26, 2016

Extend #470

  • Include PSF in tests
  • Include EDISP in tests
  • Include DataStoreObservation in tests

It also add an example in HGPSCatalog how to use the new info string scheme
see #492

@joleroi joleroi changed the title from update HESS tests to WIP: update HESS tests Feb 26, 2016

@joleroi

This comment has been minimized.

Contributor

joleroi commented Feb 26, 2016

At the moment gammapy cannot read the PA 3gauss PSF.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 29, 2016

@joleroi - There is no 3gauss for PA, only King profile.

The loading has to be changed to go either via hdu_class or hdu_type.
I could do this in #457

@cdeil cdeil added the tests label Feb 29, 2016

@cdeil cdeil added this to the 0.4 milestone Feb 29, 2016

@joleroi

This comment has been minimized.

Contributor

joleroi commented Feb 29, 2016

Will #457 be ready soon?

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 29, 2016

Will #457 be ready soon?

I hadn't planned on working on it today, but if it's a blocker for this, I can make it a priority.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Feb 29, 2016

Either that or I implement the modified loading in this PR

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 15, 2016

@joleroi - If you want to continue with this, it needs to be rebased.

Note that I've added some tests whether loading works for HAP-HD and PA here:
https://github.com/gammapy/gammapy/blob/master/gammapy/data/tests/test_data_store.py

Do you think this is OK, or is that already duplication with the "chain" tests you've set up?
Feel free to remove tests that are clearly duplicates.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

This branch has conflicts that must be resolved
Use the command line to resolve conflicts before continuing.

No need to point this out in every PR 😉

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

I am about to merge the 'chain' test and the tests you added in https://github.com/gammapy/gammapy/pull/457/files

the obs.info() takes a file as argument and print directly to that file (or sys.stdout). So far the info function in gammapy have always returned a string. I am using this to check for changes in the stores. Why did you switch to the new info scheme, and is there a way to retrieve the info as string other than writing to a temp file?

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

Ok, forget the last question. Google knows ..

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 16, 2016

We could add such a context manager as a helper to capture stdout:
http://stackoverflow.com/a/10743550/498873

Concerning info as a string versus printing, I'm undecided.
This is nicer for interactive use

>>> spam.info()

than this:

>>> print(spam.info)

Sometimes one has options what to print:

>>> spam.info(verbose=42)
>>> print(spam.info(verbose=42))

But yes, for the use case of wanting a string, having to capture it via a context manager is more cumbersome ...

I think in the few cases in Astropy .info prints (e.g. HDUList.info())?

@joleroi joleroi force-pushed the joleroi:irf_tests branch from 4713902 to 4707279 Mar 16, 2016

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

Can I merge if the tests pass, or do you want to have a look?

@joleroi joleroi changed the title from WIP: update HESS tests to Update HESS chains tests Mar 16, 2016

# Todo : Find a better way to capture stdout
# see http://stackoverflow.com/questions/5136611/capture-stdout-from-a-script-in-python/10743550#10743550
import sys
from StringIO import StringIO

This comment has been minimized.

@cdeil

cdeil Mar 16, 2016

Member

This doesn't work in Python 3:

$ python3 -c 'import StringIO'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named 'StringIO'

Also: this should really be done in a with statement, which guarantees that sys.stdout is restored when it exists.

Bottom line: you have to put this in gammapy.utils and add a test that captures something that's printed to show that it works on Python 2 and 3.
Let me know if you want me to add this.

This comment has been minimized.

@joleroi

joleroi Mar 16, 2016

Contributor

Somehow I had the feeling I wouldn't get away with that ... I will try to add a context manager

# TODO: Which target was observed?
# TODO: print info about available HDUs for this observation ...
print(str(self), file=file)

This comment has been minimized.

@joleroi

joleroi Mar 16, 2016

Contributor

@cdeil Are you ok with this solution?

In [6]: obs
Out[6]: <gammapy.data.data_store.DataStoreObservation at 0x7f7aaf1dc810>

In [7]: print obs
Info for OBS_ID = 23523
- Start time: 53343.921875 s
- Pointing pos: <SkyCoord (ICRS): (ra, dec) in deg
    (83.6333313, 21.51444435)>
- Observation duration: 1687.0 s
- Dead-time fraction: 6.240 %

In [8]: str(obs)
Out[8]: 'Info for OBS_ID = 23523\n- Start time: 53343.921875 s\n- Pointing pos: <SkyCoord (ICRS): (ra, dec) in deg\n    (83.6333313, 21.51444435)>\n- Observation duration: 1687.0 s\n- Dead-time fraction: 6.240 %'

In [9]: obs.info()
Info for OBS_ID = 23523
- Start time: 53343.921875 s
- Pointing pos: <SkyCoord (ICRS): (ra, dec) in deg
    (83.6333313, 21.51444435)>
- Observation duration: 1687.0 s
- Dead-time fraction: 6.240 %

joleroi added some commits Mar 16, 2016

@@ -907,6 +907,29 @@ putting this in ``docs/conf.py`` can also help sometimes::
warnings.showwarning = warn_with_traceback
Providing summary info for class instances

This comment has been minimized.

@joleroi

joleroi Mar 16, 2016

Contributor

@cdeil : ok?

This comment has been minimized.

@cdeil

cdeil Mar 16, 2016

Member

Maybe a shorter section title that fits in the sidebar?
E.g. "Object summary info string"?

.. code-block:: python
class foo(object):

This comment has been minimized.

@cdeil

cdeil Mar 16, 2016

Member

In Python examples shouldn't use "foo" and "bar", but "spam" and "ham":
https://youtu.be/anwy2MPT5RE
I.e. class Spam with attribute ham.

__str__ has to return ss

In this case the ``__str__`` method should be a call to ``summary`` with default
parameters. Do not use an ``info`` method, since this would lead to conflicts
for some classes in Gammapy (e.g. classes that inherit the ``info`` method from
``astropy.table.Table``.

This comment has been minimized.

@cdeil

cdeil Mar 16, 2016

Member

I think the main case where we have a lot and complex text representation is:
https://gammapy.readthedocs.org/en/latest/api/gammapy.catalog.SourceCatalogObjectHGPS.html

I think SourceCatalogObjectHGPS.print_info should become SourceCatalogObjectHGPS.summary with a multiple-choice option info = {'basic', 'associations', ...} and the other methods become private (by adding an underscore)?

An example is always better than a text description, so in this case I'd suggest to link to SourceCatalogObjectHGPS.summary from here.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

I updated the developper HOWTO and added the HGSP catalog as example. I can also go through gammapy and make everything consistent but I would prefer to do this in another PR.

@joleroi joleroi added the docs label Mar 16, 2016

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 16, 2016

@joleroi - Thanks!

Feel free to make an issue and leaving other changes to make this consistent to the future.
Then anyone can pick it up later.
You can also add a label "package-novice", it's a good first pull request to learn about docs and tests.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Mar 16, 2016

The HGPS catalog is probably not under test, so I might have broken something. I can manually test it if you tell me how to get the catalog.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 16, 2016

No worries ... I'll test the HGPS catalog after you merge (I'm using it today) and fix if something is broken in master.

joleroi added a commit that referenced this pull request Mar 16, 2016

Merge pull request #473 from joleroi/irf_tests
Update HESS chains tests

@joleroi joleroi merged commit b494e1f into gammapy:master Mar 16, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joleroi joleroi deleted the joleroi:irf_tests branch Mar 16, 2016

@cdeil cdeil changed the title from Update HESS chains tests to Improve tests for HESS data Apr 20, 2016

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