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

Add method to read bright star catalog and find bright stars in sky region #1105

Merged
merged 20 commits into from
Jul 18, 2019

Conversation

moritzhuetten
Copy link
Member

added a new 'astro' class in ctapipe.utils with a get_bright_stars() method, which returns the Yale bright star catalog (read from ctapipe-extra) into an astropy table, or with arguments
get_bright_stars(pointing = astropy.SkyCoord(), radius = astropy.Angle(), magnitude_cut = float)
selects stars brighter than the chosen magnitude in a region of given radius around a sky position

@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #1105 into master will increase coverage by 0.05%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
+ Coverage   84.48%   84.53%   +0.05%     
==========================================
  Files         182      184       +2     
  Lines       11220    11274      +54     
==========================================
+ Hits         9479     9531      +52     
- Misses       1741     1743       +2
Impacted Files Coverage Δ
ctapipe/utils/__init__.py 100% <100%> (ø) ⬆️
ctapipe/utils/tests/test_astro.py 100% <100%> (ø)
ctapipe/utils/astro.py 95.65% <95.65%> (ø)
ctapipe/tools/info.py 82.17% <0%> (-1%) ⬇️
ctapipe/image/tests/test_cleaning.py 100% <0%> (ø) ⬆️
ctapipe/image/cleaning.py 96.29% <0%> (+0.06%) ⬆️

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 62df71b...073be5b. Read the comment docs.

ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/__init__.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
maxnoe
maxnoe previously approved these changes Jul 15, 2019
@@ -6,6 +6,7 @@
from .unstructured_interpolator import UnstructuredInterpolator
from .datasets import (find_all_matching_datasets, get_table_dataset, get_dataset_path,
find_in_path)
from .astro import get_bright_stars
Copy link
Member

Choose a reason for hiding this comment

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

Add this method to __all__

Copy link
Member

Choose a reason for hiding this comment

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

This is still missing!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still missing!

Now added

ctapipe/utils/astro.py Outdated Show resolved Hide resolved
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

ok, so what's now missing is a test.

E.g. just loading the full catalog, loading a catalog with radius and pointing (e.g. pointing is crab and check that ceta tauri is there) and max magnitude (e.g. 3.5 and check that only zeta tauri is there)

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes! Looks good now. The only think missing is that it does not appear in the API documentation anywhere, so you need to add something to docs/ctapipe_api/utils/index.rst (perhaps just a section on the new astro module and an automodapi directive for it). The easiest thing is to add a docstring at the top of the astro.py with some overall explaination of the module, and just add:

.. automodapi:: ctapipe.utils.astro
    :no-inheritance-diagram:

under the "Reference/API" section of the index.rst.

Then run "make doc" to see if it looks ok.

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

One more request: besides the documentation, you also need to add a unit test in utils/tests. So far no test covers this function.

@maxnoe
Copy link
Member

maxnoe commented Jul 16, 2019

Sorry, one more thing. I have a distaste for submodules called utils . They tend to acquire random stuff over time.

Is there a better place for this? E.g. the coordinates submodule?

@moritzhuetten
Copy link
Member Author

Sorry, one more thing. I have a distaste for submodules called utils . They tend to acquire random stuff over time.

Is there a better place for this? E.g. the coordinates submodule?

Fine with me, if you prefer (or any other place). In any case, my idea for an 'astro' submodule was to gather there all astronomy-related stuff (which astropy or other modules) can not provide. For example, I discussed with @noahhdf last week whether potentially this could also a place to add some dark-patch-finder or selector (according to day, moon distance,...) or similar helpers. But that's another story, of course.

@maxnoe
Copy link
Member

maxnoe commented Jul 16, 2019

dark-patch-finder or selector (according to day, moon distance,...) or similar helpers. But that's another story, of course.

I build something like this some time ago for FACT using the hipparcos star catalogue, but haven't used it in at least two years since e don't do ratescans where the shifter selects the spot anymore.

https://github.com/fact-project/darkspot

It uses ephem and self coded transformation, wasn't used to astropy at the time.

@kosack
Copy link
Contributor

kosack commented Jul 16, 2019

We can probably make another issue or PR to deprecate ctapipe.utils and move the things inside to new places. I agree "utils" is really a catch-all for unrelated things.

@kosack
Copy link
Contributor

kosack commented Jul 16, 2019

I updated ctapipe-extra and released a new package v0.2.18 that should work with your tests. Let's see if it works.

@moritzhuetten
Copy link
Member Author

I updated ctapipe-extra and released a new package v0.2.18 that should work with your tests. Let's see if it works.

Hi Karl, thanks a lot!
however, when Travis runs it fails with

>       table = get_bright_stars(pointing, radius=2. * u.deg, magnitude_cut=3.5)
/home/travis/build/cta-observatory/ctapipe/ctapipe/utils/tests/test_astro.py:17: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/travis/build/cta-observatory/ctapipe/ctapipe/utils/astro.py:40: in get_bright_stars
    catalog = get_dataset_path("yale_bright_star_catalog5.fits.gz")
/home/travis/build/cta-observatory/ctapipe/ctapipe/utils/datasets.py:143: in get_dataset_path
    return ctapipe_resources.get(filename)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
resource_name = 'yale_bright_star_catalog5.fits.gz'
    def get(resource_name):
        """ get the filename for a resource """
        if not pkg_resources.resource_exists(__name__, resource_name):
            raise FileNotFoundError("Couldn't find resource: '{}'"
>                                   .format(resource_name))
E           FileNotFoundError: Couldn't find resource: 'yale_bright_star_catalog5.fits.gz'

Any idea why it doesn't find the file?

Thanks and cheers
Moritz

@maxnoe
Copy link
Member

maxnoe commented Jul 16, 2019

You need to increase the version number of ctapipe-extra in the yml environment files.

@kosack
Copy link
Contributor

kosack commented Jul 16, 2019

yes, just update environment.yml, and py3.6_env.yml, py3.7_env.yml and change the line with ctapipe-extra to:

 - ctapipe-extra=0.2.18

That should fix it.

kosack
kosack previously approved these changes Jul 16, 2019
ctapipe/utils/astro.py Outdated Show resolved Hide resolved
@kosack kosack dismissed their stale review July 16, 2019 14:59

clicked approve by mistake - please see comments

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

use get_table_dataset() (see comments above)

kosack
kosack previously approved these changes Jul 16, 2019
@kosack
Copy link
Contributor

kosack commented Jul 16, 2019

Once you update the yml files it should be fine.

@moritzhuetten
Copy link
Member Author

Once you update the yml files it should be fine.

And I am just updating the API doc...

@moritzhuetten
Copy link
Member Author

And I am just updating the API doc...

Should be done now!

@maxnoe
Copy link
Member

maxnoe commented Jul 16, 2019

You forgot the two other yaml files (py3.6 and py3.7), unfortunately we need all three of them

@moritzhuetten
Copy link
Member Author

You forgot the two other yaml files (py3.6 and py3.7), unfortunately we need all three of them

Added!

@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #1105 into master will increase coverage by 1.19%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
+ Coverage   84.48%   85.67%   +1.19%     
==========================================
  Files         182      185       +3     
  Lines       11220    11451     +231     
==========================================
+ Hits         9479     9811     +332     
+ Misses       1741     1640     -101
Impacted Files Coverage Δ
ctapipe/utils/__init__.py 100% <100%> (ø) ⬆️
ctapipe/utils/tests/test_astro.py 100% <100%> (ø)
ctapipe/utils/astro.py 95% <95%> (ø)
ctapipe/reco/tests/test_hillas_intersection.py 96.72% <0%> (-3.28%) ⬇️
ctapipe/utils/unstructured_interpolator.py 62.85% <0%> (-1.85%) ⬇️
ctapipe/tools/info.py 82.17% <0%> (-1%) ⬇️
ctapipe/image/tests/test_cleaning.py 100% <0%> (ø) ⬆️
ctapipe/reco/tests/test_reconstruction_methods.py 90.9% <0%> (ø)
... and 9 more

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 62df71b...886588c. Read the comment docs.

kosack
kosack previously approved these changes Jul 17, 2019
ctapipe/utils/__init__.py Outdated Show resolved Hide resolved
@kosack kosack merged commit 5a1a6d4 into cta-observatory:master Jul 18, 2019
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Aug 12, 2019
* upstream/master:
  Reduce numba signatures to improve import time (cta-observatory#1108)
  Remove unused utils (cta-observatory#1112)
  Add method to read bright star catalog and find bright stars in sky region (cta-observatory#1105)
  Add Fields to calibration containers (cta-observatory#1111)
  Reconstruction fix (also for Divergent Pointing)  (cta-observatory#946)
  updated AUTHORS with latest mailmap
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.

4 participants