Proper motion #2625

ctoennis · 2024-10-14T10:32:17Z

This pull request adds a utility to get star positions with proper motion from different catalogues. It allows to cache the catalogue lookup locally and uses cached data before looking up data online again.

mexanick · 2024-10-14T12:03:53Z

src/ctapipe/utils/astro.py

+CACHE_FILE = Path("~/.psf_stars.ecsv").expanduser()
+
+
+def cut_stars(stars, pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None):


I suggest to use "select_stars" as a function name. "cut" is a jargon.

mexanick · 2024-10-14T12:08:32Z

src/ctapipe/utils/astro.py

 from astropy.coordinates import Angle, SkyCoord
+from astropy.table import Table
+from astropy.time import Time
+from astroquery.vizier import Vizier


This is a new dependency. It should be included in pyproject.
@maxnoe is it ok to have it? Or we need to download and stash a cached version of the catalog?

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.

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?
Schould we then have multiple files uploaded for different catalogues?

mexanick · 2024-10-14T12:08:41Z

src/ctapipe/utils/astro.py

+    return stars
+
+
+def get_bright_stars_with_motion(


Do we have a reason to keep the getter without proper motion?

No, that's essentially a bug

ok, i'll get rid of it

mexanick · 2024-10-14T12:09:16Z

src/ctapipe/utils/astro.py

+        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


Create an enum with human-readable catalog names.

i made an enum with 3 catalogues. let me know if i should add any specific catalog that would be useful

mexanick · 2024-10-14T12:10:45Z

src/ctapipe/utils/tests/test_astro.py

+
+
+def test_get_bright_stars_with_motion():
+    """


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.

oka, i will get to that part tomorrow morning

ctoennis · 2024-10-15T12:53:13Z

I extended the tests to check if stars are moving and to check both catalogues. Also i added to the cached metadata which catalog was used. Right now the function will download the catalog data again if you use a different one.

I was thinking to add to the cached file some part of the filename that marks the catalog used, so that you can cache multiple catalogs.

ctoennis · 2024-10-16T08:54:08Z

For the startracker code each star will need an identifier and not all stars have nice common names available. I suggest we use the HIP number for identification as it is the most available identifier for the objects we can use for pointing and PSF fitting.

maxnoe

I left some comments in-line.

The main point is that the star catalog(s) should remain a resource file for users and the code to create it is run by the developers when we need to update it.

maxnoe · 2024-10-18T09:04:06Z

docs/changes/2625.features.rst

@@ -0,0 +1,4 @@
+Add ''get_bright_stars_with_motion'' to ctapipe.utils.astro


Please update the changelog once we have this finalized

maxnoe · 2024-10-18T09:06:45Z

src/ctapipe/utils/astro.py


 __all__ = ["get_bright_stars"]


-def get_bright_stars(pointing=None, radius=None, magnitude_cut=None):
+CACHE_FILE = Path("~/.psf_stars.ecsv").expanduser()


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

def get_cache_path(url, cache_name="ctapipe", env_override="CTAPIPE_CACHE"):

instead of just storing a hidden file in the user's home

maxnoe · 2024-10-18T09:19:43Z

src/ctapipe/utils/astro.py


-    This function is using the Yale bright star catalog, available through ctapipe
-    data downloads.
+    if Bmag_cut is not None and "Bmag" in stars.keys():


Are Bmag / BTmag and Vmag / VTmag really the same?

Why was it needed to add the "T" versions?

some catalogs only have the T versions available

But we only deal with two here, which to my knowledge have Bmag and Vmag

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:

https://vizier.cds.unistra.fr/viz-bin/VizieR-3?-source=V/50&-out.max=50&-out.form=HTML%20Table&-out.add=_r&-out.add=_RAJ,_DEJ&-sort=_r&-oc.form=sexa

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.

maxnoe · 2024-10-18T09:20:35Z

src/ctapipe/utils/astro.py

+            "pmDE",
+            "Vmag",
+            "Bmag",
+            "BTmag",


Why also the T versions?

maxnoe · 2024-10-18T09:22:46Z

src/ctapipe/utils/astro.py

+        catalog=StarCatalogues[catalog].value,
+        columns=[
+            "recno",
+            "RAJ2000",


Please remove the computed "RAJ2000" / "DEJ2000" values.

These are computed by vizier on the fly and are redundant with the original coordinates and the proper motions.

We anyway need to apply proper motion, so it doesn't matter whether we start from the catalog epoch or j2000

maxnoe · 2024-10-18T09:26:07Z

src/ctapipe/utils/astro.py

+    stars = vizier.query_constraints(Vmag="0.0..10.0")[0]
+
+    if "Bmag" not in stars.keys():
+        log.exception("The chosen catalog does not have Bmag data")


This is wrong usage of log.exception. log.exception is meant only to be used in an except block to provide additional information to the traceback:
https://docs.python.org/3/library/logging.html#logging.Logger.exception

This should be an actual exception, so throw a KeyError

maxnoe · 2024-10-18T09:27:50Z

src/ctapipe/utils/astro.py

+        log.exception("The chosen catalog does not have Vmag data")
+    stars.meta["Catalog"] = StarCatalogues[catalog].value
+
+    stars.write(CACHE_FILE, overwrite=True)


THis should use the (at the moment unused) filename option.

But I would completely remove the filename option and just return the table. Querying the catalog and writing it to a file should be separate.

maxnoe · 2024-10-18T09:28:09Z

src/ctapipe/utils/astro.py

+
+
+def get_bright_stars(
+    pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None, catalog="Yale"


catalog argument should use the enum

maxnoe · 2024-10-18T09:28:20Z

src/ctapipe/utils/astro.py

+
+def get_bright_stars(
+    pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None, catalog="Yale"
+):  # max_magnitude):


remove commented part

For backwards compatibilty, we could think about keeping the max_magnitude argument and adding an magnitude_type= keyword that can either be "Vmag" or "BMag"

maxnoe · 2024-10-18T09:33:11Z

src/ctapipe/utils/astro.py

-        copy=False,
-    )
-    catalog["ra_dec"] = starpositions
+    if CACHE_FILE.exists():


This should use the importlib resources system to load the resources file for the specified catalog

maxnoe · 2024-10-18T10:30:32Z

src/ctapipe/utils/astro.py

+
+
+class StarCatalogues(Enum):
+    Yale = "V/50/catalog"  # Yale bright star catalogue


Change the comment to be above and have the form #: , this will be picked up by sphinx for the docs

Christoph Toennis added 4 commits October 11, 2024 12:26

I added the HIP catalog lookup with proper motion

2c4f4cf

refactoring

3be8ede

updated docustring

704f7a0

Allowed to use different catalogs, added documentation

12c49f0

ctoennis requested a review from maxnoe October 14, 2024 10:32

ctoennis self-assigned this Oct 14, 2024

ctoennis requested a review from mexanick October 14, 2024 10:32

ctoennis added coordinates calibration new functionality labels Oct 14, 2024

Christoph Toennis added 2 commits October 14, 2024 12:38

adding change notes

8de6331

Fixed change notes filename

0ce70b7

mexanick reviewed Oct 14, 2024

View reviewed changes

Christoph Toennis added 3 commits October 14, 2024 15:58

Implementing requested changes by mysha

0ca8561

Removing old getter

4f28af2

Extending tests and adding metadata to cache file

ced74cc

Made a function to create star table

80dfc87

Fixing an issue with the catalogues

0d6c5a4

maxnoe requested changes Oct 18, 2024

View reviewed changes

maxnoe reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper motion #2625

Proper motion #2625

ctoennis commented Oct 14, 2024

mexanick Oct 14, 2024

mexanick Oct 14, 2024

maxnoe Oct 15, 2024

ctoennis Oct 16, 2024

mexanick Oct 14, 2024

maxnoe Oct 14, 2024

ctoennis Oct 14, 2024

mexanick Oct 14, 2024

ctoennis Oct 14, 2024

mexanick Oct 14, 2024

ctoennis Oct 14, 2024

ctoennis commented Oct 15, 2024

ctoennis commented Oct 16, 2024

maxnoe left a comment

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

ctoennis Oct 18, 2024

maxnoe Oct 18, 2024

ctoennis Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

maxnoe Oct 18, 2024

		CACHE_FILE = Path("~/.psf_stars.ecsv").expanduser()


		def cut_stars(stars, pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None):

		@@ -0,0 +1,4 @@
		Add ''get_bright_stars_with_motion'' to ctapipe.utils.astro



		def get_bright_stars(
		pointing=None, radius=None, Bmag_cut=None, Vmag_cut=None, catalog="Yale"



		class StarCatalogues(Enum):
		Yale = "V/50/catalog" # Yale bright star catalogue

Proper motion #2625

Are you sure you want to change the base?

Proper motion #2625

Conversation

ctoennis commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctoennis commented Oct 15, 2024

ctoennis commented Oct 16, 2024

maxnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment