Add Harris Globular Cluster catalog loader#384
Conversation
|
Hi @casazza this is looking great! One thing that seems to be missing is the Please go ahead and run the catalog_imports.main module to generate the pifinder_objects.db file and add it along with this PR. |
brickbots
left a comment
brickbots
left a comment
There was a problem hiding this comment.
A few typing issues here, which can be pretty annoying. I think only the 'any' vs 'Any' will cause issues and need to be fixed before this can be merged, but please let me know if this is getting too in the weeds for you and we can merge this and I'll fix up any type related issues
| for (start, end), (_, field_dtype) in zip(col_specs, dtype) | ||
| ) | ||
|
|
||
| def parse_field(value: str, field_dtype: str) -> any: |
There was a problem hiding this comment.
Your type annotations are really welcome.... but they are using some typing features from newer versions of python and are failing the mypy tests and will likely error in 3.9. For 3.9 compatibility can use:
from typing import Any
and use 'Any' as the return value 👍
| Returns: | ||
| dict with ra, dec, mag, size, catalog_names, common_names, description, primary_name | ||
| """ | ||
| result: Dict[str, any] = {} |
There was a problem hiding this comment.
Here is another place you can swap for Any
| result["primary_name"]: str = f"Har {seq}" | ||
|
|
||
| # Process catalog ID (first field) | ||
| cluster_id = entry["ID"].strip() |
There was a problem hiding this comment.
I think you might want entry["ID"].item().strip() here?
| logging.info("Loading Harris Globular Cluster catalog") | ||
| catalog: str = "Har" | ||
| obj_type: str = "Gb" # Globular Cluster | ||
| conn, _ = objects_db.get_conn_cursor() |
There was a problem hiding this comment.
Hmmm.. mypy is going to complain a bit here due to the way that objects_db is defined/initialized. This is not really a problem that you want to fix in this PR (unless you do!) so I'd suggest leaving this function untyped to avoid mypy checking it
If you remove the -> None: from the function definition, it will be an untyped function and mypy will happily ignore it 👍
|
Thanks for the tips. I was looking at the findings. I will see if I can
make "nox" happy.
The pifinder_objects.db will be a problem. I can't load 3 catalogs, but I
can load mine. I really don't want to test on my PF. So I can't build a
full pifinder_objects.db file.
Mark Casazza
http://casazza.net
Home of the Clear Sky Alarm Clock and Tonight's Sky
…On Tue, Feb 17, 2026 at 6:47 PM Richard ***@***.***> wrote:
***@***.**** commented on this pull request.
A few typing issues here, which can be pretty annoying. I think only the
'any' vs 'Any' will cause issues and need to be fixed before this can be
merged, but please let me know if this is getting too in the weeds for you
and we can merge this and I'll fix up any type related issues
------------------------------
In python/PiFinder/catalog_imports/harris_loader.py
<#384 (comment)>:
> + ("X", "f4"), # X component
+ ("Y", "f4"), # Y component
+ ("Z", "f4"), # Z component
+ ("Vt", "f4"), # Integrated V magnitude
+ ("MVt", "f4"), # Absolute magnitude
+ ("Rc", "f4"), # Core radius (arcmin)
+ ("Rh", "f4"), # Half-mass radius (arcmin)
+ ]
+
+ def parse_line(line: str) -> tuple:
+ return tuple(
+ parse_field(line[start:end].strip(), field_dtype)
+ for (start, end), (_, field_dtype) in zip(col_specs, dtype)
+ )
+
+ def parse_field(value: str, field_dtype: str) -> any:
Your type annotations are really welcome.... but they are using some
typing features from newer versions of python and are failing the mypy
tests and will likely error in 3.9. For 3.9 compatibility can use:
from typing import Any
and use 'Any' as the return value 👍
------------------------------
In python/PiFinder/catalog_imports/harris_loader.py
<#384 (comment)>:
> + # Not an official catalog - treat as common name
+ return False, name
+
+
+def create_cluster_object(entry: npt.NDArray, seq: int) -> Dict[str, any]:
+ """
+ Create a single cluster object from catalog entry.
+
+ Args:
+ entry: numpy structured array row with cluster data
+ seq: sequence number (line number in catalog)
+
+ Returns:
+ dict with ra, dec, mag, size, catalog_names, common_names, description, primary_name
+ """
+ result: Dict[str, any] = {}
Here is another place you can swap for Any
------------------------------
In python/PiFinder/catalog_imports/harris_loader.py
<#384 (comment)>:
> + description_parts.append(f"Absolute magnitude: {mvt:.2f}")
+
+ result["description"] = "\n".join(description_parts) if description_parts else ""
+
+ if VERBOSE and description_parts:
+ logging.debug(f" Description: {len(description_parts)} features")
+
+ # Separate catalog names from common names
+ # Official catalogs: NGC, IC, M, C, Col, Ta2, H, SaA, SaM, SaR, Str, EGC, RDS, B, Sh2, Abl, Arp, TLK, WDS
+ # Everything else (Pal, AM, Terzan, etc.) becomes a common name
+ result["catalog_names"]: List[str] = [] # For aka_names (catalog designations)
+ result["common_names"]: List[str] = [] # For insert_name (common names)
+ result["primary_name"]: str = f"Har {seq}"
+
+ # Process catalog ID (first field)
+ cluster_id = entry["ID"].strip()
I think you might want entry["ID"].item().strip() here?
------------------------------
In python/PiFinder/catalog_imports/harris_loader.py
<#384 (comment)>:
> + # Log summary
+ if VERBOSE:
+ logging.info(
+ f" Primary: {result['primary_name']}, "
+ f"Catalog names: {len(result['catalog_names'])}, "
+ f"Common names: {len(result['common_names'])}"
+ )
+
+ return result
+
+
+def load_harris() -> None:
+ logging.info("Loading Harris Globular Cluster catalog")
+ catalog: str = "Har"
+ obj_type: str = "Gb" # Globular Cluster
+ conn, _ = objects_db.get_conn_cursor()
Hmmm.. mypy is going to complain a bit here due to the way that objects_db
is defined/initialized. This is not really a problem that you want to fix
in this PR (unless you do!) so I'd suggest leaving this function untyped to
avoid mypy checking it
If you remove the -> None: from the function definition, it will be an
untyped function and mypy will happily ignore it 👍
—
Reply to this email directly, view it on GitHub
<#384 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AK3D3Q7DZ732OJECFG63QF34MOSALAVCNFSM6AAAAACVOWDW76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQMJWHA2DMNJVGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
casazza
left a comment
casazza
left a comment
There was a problem hiding this comment.
Third times the charm.
Unit test updated to expect 20 catalogs
harris_loader corrected to correct catalog sources and remove spaces
corrected database
|
Thank you @casazza !!!! |
2 participants
Second attempt to deliver a pull request with the 8 files involved in adding the Harris Globular Catalog to PiFinder.