+^ look_in_catalog avoid unrelated keys for cat_type

[radonnachie]

Description

The look_in_catalog function never considered the cat_type when accessing keys, leading to keys unrelated to the cat_type being hard-accessed.

Motivation and Context

When adding multiple UV objects, the look_in_catalog hit an error trying to access the "cat_times" key within the phase_catalog dict-entry. The entries are all of "sidereal" type though, which isn't expected to have a "cat_times" key.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change (documentation changes only)
• Version change
• Build or continuous integration change

Checklist:

• I have read the contribution guide.
• My code follows the code style of this project.

Bug fix checklist:

• My fix includes a new test that breaks as a result of the bug (if possible).
• All new and existing tests pass.
• I have updated the CHANGELOG.

New feature checklist:

• I have added or updated the docstrings associated with my feature using the numpy docstring format.
• I have updated the tutorial to highlight my new feature (if appropriate).
• I have added tests to cover my new feature.
• All new and existing tests pass.
• I have updated the CHANGELOG.

Breaking change checklist:

• I have updated the docstrings associated with my change using the numpy docstring format.
• I have updated the tutorial to reflect my changes (if appropriate).
• My change includes backwards compatibility and deprecation warnings (if possible).
• I have added tests to cover my changes.
• All new and existing tests pass.
• I have updated the CHANGELOG.

Documentation change checklist:

• Any updated docstrings use the numpy docstring format.
• If this is a significant change to the readme or other docs, I have checked that they are rendered properly on ReadTheDocs. (you may need help to get this branch to build on RTD, just ask!)

Version change checklist:

• I have updated the CHANGELOG to put all the unreleased changes under the new version (leaving the unreleased section empty).
• I have noted any dependency changes since the last version and will update the conda package build accordingly.

Build or continuous integration change checklist:

• If required or optional dependencies have changed (including version numbers), I have updated the readme to reflect this.
• If this is a new CI setup, I have added the associated badge to the readme and to references/make_index.py (if appropriate).

[radonnachie]

Obviously need to run through the bug-fix checklist. Left it up to the reviewer to consider the style of the commit.

[kartographer]

Hi @radonnachie - thanks for catching this, nominally all entries inside of the phase center dictionaries are supposed to be present, although it's possible that's not actually documented in the appropriate places. Long-term there's a plan to move the catalog to an object, but for now we're trying to minimize changes to the existing stuff to make that transition easier. I think there's a much easier way to wrap the problem you've nominally come across (a 1-line replacement) which I've added below. Note that we normally also expect bug-fix PRs to include a test which fails with the older code, let me know if you need help putting that together.

[wfarah]

@kartographer for the test, how should I include the data? I can include the traceback in a second

[wfarah]

Including the full error here:

In [1]: fname1 = "LoA.C0352/uvh5_60647_62965_9760406_3c286_0001.uvh5"

In [2]: fname2 = "LoA.C0544/uvh5_60647_62965_9760406_3c286_0001.uvh5"

In [3]: from pyuvdata import UVData

In [4]: uv1 = UVData()

In [5]: uv2 = UVData()

In [6]: uv1.read_uvh5(fname1)

In [7]: uv2.read_uvh5(fname2)

In [8]: uv = uv1 + uv2
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[8], line 1
----> 1 uv = uv1 + uv2

File /opt/mnt/miniconda3/envs/pyuvdata/lib/python3.11/site-packages/pyuvdata/uvdata/uvdata.py:5548, in UVData.__add__(self, other, inplace, verbose_history, run_check, check_extra, run_check_acceptability, strict_uvw_antpos_check, ignore_name)
   5542         raise ValueError(msg)
   5544 # Begin manipulating the objects.
   5545
   5546 # First, handle the internal source catalogs, since merging them is kind of a
   5547 # weird, one-off process (i.e., nothing is cat'd across a particular axis)
-> 5548 this._consolidate_phase_center_catalogs(other=other, ignore_name=ignore_name)
   5550 # Next, we want to make sure that the ordering of the _overlapping_ data is
   5551 # the same, so that things can get plugged together in a sensible way.
   5552 if len(this_blts_ind) != 0:

File /opt/mnt/miniconda3/envs/pyuvdata/lib/python3.11/site-packages/pyuvdata/uvdata/uvdata.py:1391, in UVData._consolidate_phase_center_catalogs(self, reference_catalog, other, ignore_name)
   1386 for cat_id in list(reference_catalog):
   1387     # Normally one would wrap this in an items() call above, except that for
   1388     # testing it's sometimes convenient to use self.phase_center_catalog as
   1389     # the ref catalog, which causes a RunTime error due to updates to the dict.
   1390     cat_entry = reference_catalog[cat_id]
-> 1391     match_id, match_diffs = utils.phase_center_catalog.look_in_catalog(
   1392         self.phase_center_catalog, phase_dict=cat_entry, ignore_name=ignore_name
   1393     )
   1394     if match_id is None or match_diffs != 0:
   1395         # If no match, just add the entry
   1396         self._add_phase_center(cat_id=cat_id, **cat_entry)

File /opt/mnt/miniconda3/envs/pyuvdata/lib/python3.11/site-packages/pyuvdata/utils/phase_center_catalog.py:200, in look_in_catalog(phase_center_catalog, cat_name, cat_type, cat_lon, cat_lat, cat_frame, cat_epoch, cat_times, cat_pm_ra, cat_pm_dec, cat_dist, cat_vrad, ignore_name, target_cat_id, phase_dict)
    193                 cat_diffs += not np.allclose(
    194                     phase_dict[key],
    195                     check_dict[key],
    196                     tol_dict[key][0],
    197                     tol_dict[key][1],
    198                 )
    199     else:
--> 200         cat_diffs += check_dict[key] is not None
    201 if (cat_diffs == 0) or (cat_name == name):
    202     if cat_diffs < match_diffs:
    203         # If our current match is an improvement on any previous matches,
    204         # then record it as the best match.

KeyError: 'cat_times'

[radonnachie]

I'll just setup a test with simple dicts that emulate the scenario.

[radonnachie]

I'm thinking that instead of testing that look_in_catalog handles the ps_catalogs correctly that the test should directly add 2 sidereal UVData objects... would any of the existing data files support this test @kartographer?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (9317875) to head (712fdff).
Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1506   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          63       63           
  Lines       21834    21834           
=======================================
  Hits        21819    21819           
  Misses         15       15           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[radonnachie]

I tried adding a test. Not sure how robust it is, hopefully serves as an indication of what we're struggling with.

[kartographer]

@radonnachie -- it looks like the test is actually failing for unrelated reasons here, but let me see if I can spin up a simple fix...

[kartographer]

Okay, this is a much more focused test that I've at least verified in an ipython shell fails with the old code and works with the new code.

[kartographer]

@kartographer for the test, how should I include the data? I can include the traceback in a second

@wfarah - just to clarify here, we usually just want to see a test that should fail in the old code. We don't necessarily need to see a traceback (though it is helpful sometimes!), but usually during review whoever is looking at the PR will at least make sure that the test appears to cover the nominal scenario.

[bhazelton]

Looks like it's failing the pre-commit linting check. (The warning test failure and hera_cal failures are unrelated known issues and are not required to pass). There are directions in the readme to install the pre-commit hook to prevent pushing code that will fail the CI.

[kartographer]

Looking good -- just one thing missing: an entry in the change log =)

[radonnachie]

Right, I've added a somewhat verbose and cumbersome entry to the changelog. Trust it's good enough...

[kartographer]

One more minor fix!

[kartographer]

Done -- thanks @radonnachie!

[radonnachie]

Thank you for your help and patience.
It's not fully solve our issue, so I'll be back before long with another PR. I'll aim to make it a bit more seamless.

[kartographer]

@radonnachie - can you put in an issue so that we can follow along (and potentially help)?