-
Notifications
You must be signed in to change notification settings - Fork 269
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
Proper motion #2625
base: main
Are you sure you want to change the base?
Proper motion #2625
Changes from 6 commits
2c4f4cf
3be8ede
704f7a0
12c49f0
8de6331
0ce70b7
0ca8561
4f28af2
ced74cc
80dfc87
0d6c5a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Add ''get_bright_stars_with_motion'' to ctapipe.utils.astro | ||
this function allows to look up the position of stars | ||
around a specified location including proper motion. | ||
different catalogues can be used. |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -4,10 +4,151 @@ | |||
not provided by external packages and/or to satisfy particular needs of | ||||
usage within ctapipe. | ||||
""" | ||||
from astropy import units as u | ||||
|
||||
import logging | ||||
from pathlib import Path | ||||
|
||||
import astropy.units as u | ||||
from astropy.coordinates import Angle, SkyCoord | ||||
from astropy.table import Table | ||||
from astropy.time import Time | ||||
from astroquery.vizier import Vizier | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a new dependency. It should be included in pyproject. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer having the catalog we need as a resource file, it should be fairly small. It would be good have the code to create it here in any case, but that can be done without hard-requiring astroquery. it should be kept as an optional dependency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i made a function to make the catalog file in the last commit and added astroquery to the optional dependencies. The function get_bright_stars() should then only look to the resource file with the function get_table_dataset(), no? |
||||
|
||||
log = logging.getLogger("main") | ||||
|
||||
__all__ = ["get_bright_stars_with_motion", "get_bright_stars"] | ||||
|
||||
|
||||
CACHE_FILE = Path("~/.psf_stars.ecsv").expanduser() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be changed. As discussed in the comments, we want to have the catalogs as resource files in the package and only have the code to create them for developers to update those files if needed. So this should not be needed at all. if we need it, it should be using this function here to obtain an appropriate cache path: ctapipe/src/ctapipe/utils/download.py Line 69 in 40faf69
instead of just storing a hidden file in the user's home |
||||
|
||||
|
||||
def cut_stars(stars, pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to use "select_stars" as a function name. "cut" is a jargon. |
||||
""" | ||||
utility function to cut stars based on magnitude and/or location | ||||
|
||||
Parameters | ||||
---------- | ||||
stars: astropy table | ||||
Table of stars, including magnitude and coordinates | ||||
pointing: astropy Skycoord | ||||
pointing direction in the sky (if none is given, full sky is returned) | ||||
radius: astropy angular units | ||||
Radius of the sky region around pointing position. Default: full sky | ||||
Vmag_cut: float | ||||
Return only stars above a given apparent magnitude. Default: None (all entries) | ||||
Bmag_cut: float | ||||
Return only stars above a given absolute magnitude. Default: None (all entries) | ||||
|
||||
Returns | ||||
------- | ||||
Astropy table: | ||||
List of all stars after cuts with same keys as the input table stars | ||||
""" | ||||
|
||||
if Bmag_cut is not None and "Bmag" in stars.keys(): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are Why was it needed to add the "T" versions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some catalogs only have the T versions available There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we only deal with two here, which to my knowledge have Bmag and Vmag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here is the link to the hippoarcos catalogue: https://vizier.cds.unistra.fr/viz-bin/VizieR-3?-source=I/239/hip_main&-out.max=50&-out.form=HTML%20Table&-out.add=_r&-out.add=_RAJ,_DEJ&-sort=_r&-oc.form=sexa It has Vmag, but no Bmag. It has BTmag and VTmag available. The Yale catalogue is here: Here it's only Vmag. the Nomad catalogue has both Bmag and Vmag: https://vizier.cds.unistra.fr/viz-bin/VizieR-2 If it you want to not have the T versions i can remove it and add error messages when you try to use Bmag with the Yale and Hippoarcos catalogues. |
||||
stars = stars[stars["Bmag"] < Bmag_cut] | ||||
if Vmag_cut is not None and "Vmag" in stars.keys(): | ||||
stars = stars[stars["Vmag"] < Vmag_cut] | ||||
|
||||
if radius is not None: | ||||
if pointing is None: | ||||
raise ValueError( | ||||
"Sky pointing, pointing=SkyCoord(), must be " | ||||
"provided if radius is given." | ||||
) | ||||
separations = stars["ra_dec"].separation(pointing) | ||||
stars["separation"] = separations | ||||
stars = stars[separations < radius] | ||||
|
||||
return stars | ||||
|
||||
|
||||
def get_bright_stars_with_motion( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a reason to keep the getter without proper motion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's essentially a bug There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, i'll get rid of it |
||||
pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None, catalog="V/50/catalog" | ||||
): # max_magnitude): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove commented part There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For backwards compatibilty, we could think about keeping the |
||||
""" | ||||
Get an astropy table of bright stars from a VizieR catalog | ||||
|
||||
You can browse the catalogs at https://vizier.cds.unistra.fr/viz-bin/VizieR | ||||
|
||||
__all__ = ["get_bright_stars"] | ||||
Parameters | ||||
---------- | ||||
pointing: astropy Skycoord | ||||
pointing direction in the sky (if none is given, full sky is returned) | ||||
radius: astropy angular units | ||||
Radius of the sky region around pointing position. Default: full sky | ||||
Vmag_cut: float | ||||
Return only stars above a given apparent magnitude. Default: None (all entries) | ||||
Bmag_cut: float | ||||
Return only stars above a given absolute magnitude. Default: None (all entries) | ||||
catalog: string | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create an enum with human-readable catalog names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i made an enum with 3 catalogues. let me know if i should add any specific catalog that would be useful |
||||
The location of the catalog to be used. The hipparcos catalog is found at I/239/hip_main. Default: V/50/catalog | ||||
|
||||
Returns | ||||
------- | ||||
Astropy table: | ||||
List of all stars after cuts with names, catalog numbers, magnitudes, | ||||
and coordinates | ||||
""" | ||||
|
||||
stars = None | ||||
|
||||
if CACHE_FILE.exists(): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use the importlib resources system to load the resources file for the specified catalog |
||||
log.info("Loading stars from cached table") | ||||
try: | ||||
stars = Table.read(CACHE_FILE) | ||||
if Bmag_cut is not None: | ||||
if stars.meta["Bmag_cut"] >= Bmag_cut: | ||||
log.debug(f"Loaded table is valid for { Bmag_cut= }") | ||||
else: | ||||
log.debug("Loaded cache table has smaller magnitude_cut, reloading") | ||||
stars = None | ||||
if Vmag_cut is not None: | ||||
if stars.meta["Vmag_cut"] >= Vmag_cut: | ||||
log.debug(f"Loaded table is valid for {Vmag_cut= }") | ||||
else: | ||||
log.debug("Loaded cache table has smaller magnitude_cut, reloading") | ||||
stars = None | ||||
except Exception: | ||||
log.exception("Cache file exists but reading failed. Recreating") | ||||
|
||||
if stars is None: | ||||
log.info("Querying Vizier for bright stars catalog") | ||||
# query vizier for stars with 0 <= Vmag <= max_magnitude | ||||
vizier = Vizier( | ||||
catalog=catalog, | ||||
columns=["HR", "RAJ2000", "DEJ2000", "pmRA", "pmDE", "Vmag"], | ||||
row_limit=1000000, | ||||
) | ||||
|
||||
stars = vizier.query_constraints(Vmag="0.0..100.0")[0] | ||||
if "Bmag" in stars.keys(): | ||||
if Bmag_cut is not None: | ||||
stars.meta["Bmag_cut"] = Bmag_cut | ||||
elif Bmag_cut is not None: | ||||
log.exception("The chosen catalog does not have Bmag data") | ||||
if "Vmag" in stars.keys(): | ||||
if Vmag_cut is not None: | ||||
stars.meta["Vmag_cut"] = Vmag_cut | ||||
elif Vmag_cut is not None: | ||||
log.exception("The chosen catalog does not have Vmag data") | ||||
|
||||
stars.write(CACHE_FILE, overwrite=True) | ||||
|
||||
stars["ra_dec"] = SkyCoord( | ||||
ra=Angle(stars["RAJ2000"], unit=u.deg), | ||||
dec=Angle(stars["DEJ2000"], unit=u.deg), | ||||
pm_ra_cosdec=stars["pmRA"].quantity, # yes, pmRA is already pm_ra_cosdec | ||||
pm_dec=stars["pmDE"].quantity, | ||||
frame="icrs", | ||||
obstime=Time("J2000.0"), | ||||
) | ||||
|
||||
stars = cut_stars( | ||||
stars, pointing=pointing, radius=radius, Bmag_cut=Bmag_cut, Vmag_cut=Vmag_cut | ||||
) | ||||
|
||||
return stars | ||||
|
||||
|
||||
def get_bright_stars(pointing=None, radius=None, magnitude_cut=None): | ||||
|
@@ -48,18 +189,9 @@ def get_bright_stars(pointing=None, radius=None, magnitude_cut=None): | |||
) | ||||
catalog["ra_dec"] = starpositions | ||||
|
||||
if magnitude_cut is not None: | ||||
catalog = catalog[catalog["Vmag"] < magnitude_cut] | ||||
|
||||
if radius is not None: | ||||
if pointing is None: | ||||
raise ValueError( | ||||
"Sky pointing, pointing=SkyCoord(), must be " | ||||
"provided if radius is given." | ||||
) | ||||
separations = catalog["ra_dec"].separation(pointing) | ||||
catalog["separation"] = separations | ||||
catalog = catalog[separations < radius] | ||||
catalog = cut_stars( | ||||
catalog, pointing=pointing, radius=radius, Vmag_cut=magnitude_cut | ||||
) | ||||
|
||||
catalog.remove_columns(["RAJ2000", "DEJ2000"]) | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,28 @@ | |
This module contains the utils.astro unit tests | ||
""" | ||
from astropy import units as u | ||
from astropy.coordinates import SkyCoord | ||
from astropy.coordinates import Angle, SkyCoord | ||
|
||
from ..astro import get_bright_stars | ||
from ..astro import get_bright_stars, get_bright_stars_with_motion | ||
|
||
|
||
def test_get_bright_stars_with_motion(): | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you add more catalogs, it makes sense to test all of them. Also, it would be nice to test a but more details, or add more comments about the expected outcome of the query. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oka, i will get to that part tomorrow morning |
||
unit test for utils.astro.get_bright_stars_with_motion(). | ||
""" | ||
pointing = SkyCoord( | ||
ra=Angle("03 47 29.1", unit=u.deg), | ||
dec=Angle("+24 06 18", unit=u.deg), | ||
frame="icrs", | ||
) | ||
|
||
# lets find 25 Eta Tau | ||
|
||
table = get_bright_stars_with_motion( | ||
pointing=pointing, radius=1.0 * u.deg, Vmag_cut=3.5 | ||
) | ||
|
||
assert len(table) == 1 | ||
|
||
|
||
def test_get_bright_stars(): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the changelog once we have this finalized