JP-2812 : Allow user-provided custom image catalogs for tweakreg

[mcara]

Resolves JP-2812

This PR enables support for user-provided image catalogs in tweakreg. This is accomplished via meta.tweakreg_catalog attribute and ignore_custom_catalog=False (this is needed for backward compatibility).

Normally users would have to open data model and set datamodel.meta.tweakreg_catalog to the file name of a catalog on disk. Alternatively, users can edit ASN table and supply catalog name there via member attribute tweakreg_catalog, e.g.,

{
    "asn_type": "None",
    "asn_rule": "DMS_Level3_Base",
    "version_id": null,
    "code_version": "1.6.4",
    "degraded_status": "No known degraded exposures in association.",
    "program": "noprogram",
    "constraints": "No constraints",
    "asn_id": "a3001",
    "target": "none",
    "asn_pool": "none",
    "products": [
        {
            "name": "resampled_simple_nis",
            "members": [
                {
                    "expname": "simple_00001_nis_cal.fits",
                    "exptype": "science",
		    "tweakreg_catalog": "custom_catalog1.ecsv"
                },
                {
                    "expname": "simple_00002_nis_cal.fits",
                    "exptype": "science",
		    "tweakreg_catalog": "custom_catalog2.ecsv"
                }
            ]
        }
    ]
}

This will modify input cal files and set datamodel.meta.tweakreg_catalog to the value specified in the ASN table.

Checklist for maintainers

• [x] added entry in CHANGES.rst within the relevant release section
• [ ] updated or added relevant tests
• [x] updated relevant documentation
• [x] added relevant milestone
• [x] added relevant label(s)
• [x] ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR
• [ ] Make sure the JIRA ticket is resolved properly

Regression test: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/414/

[codecov[bot]]

Codecov Report

Base: 79.66% // Head: 79.64% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (966f05d) compared to base (e154d67). Patch has no changes to coverable lines.

:exclamation: Current head 966f05d differs from pull request most recent head 54cf12a. Consider uploading reports for the commit 54cf12a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7022      +/-   ##
==========================================
- Coverage   79.66%   79.64%   -0.03%     
==========================================
  Files         412      412              
  Lines       37328    37322       -6     
==========================================
- Hits        29737    29724      -13     
- Misses       7591     7598       +7     

Flag	Coverage Δ
nightly	79.62% <0.00%> (-0.03%)	:arrow_down:
unit	51.98% <0.00%> (-0.01%)	:arrow_down:

Impacted Files	Coverage Δ
jwst/associations/lib/log_config.py	89.55% <0.00%> (-10.45%)	:arrow_down:
jwst/jump/jump.py	100.00% <0.00%> (ø)
jwst/jump/jump_step.py	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[mcara]

@hbushouse @nden This is ready for review. @stscieisenhamer no major changes were made since your review (mostly docs). I re-requested your review by accident.

[larrybradley]

What happens if a user provides only a catalog for some (not all) of the input images?

[mcara]

What happens if a user provides only a catalog for some (not all) of the input images?

The code would use user-provided catalogs when provided and would use internal source-finding algorithm for the images without user catalogs.

[mcara]

From doctsting:

Custom Source Catalogs

When meta.tweakreg_catalog attribute of input data models is None or an empty string, then tweakreg step will attempt to detect sources in the input images. Stars are detected in the image using the Photutils "daofind" function.

[mcara]

@nden There should be documentation, I think in a file README.rst

[nden]

@nden There should be documentation, I think in a file README.rst

Ah, yes, sorry. I missed that README is in the docs directory.

[mairanteodoro]

Looking good to me.

[mcara]

I am a big fan of ternary operators... in general. But here I feel that readability suffers a little. Not worth it, IMO. The code is doing what it needs to do.

With regard to multiple IF statements. I think I finally understood what @mairanteodoro and @nden mean. I do not think it is possible to join top two statements in this code:

for attr, val in member.items():
    if attr in RECOGNIZED_MEMBER_FIELDS:
        if attr == 'tweakreg_catalog':
            # the custom processing below applies only to 'tweakreg_catalog' attribute:
            if val.strip():
                val = op.join(asn_dir, val)
            else:
                val = None

        # we can imagine custom processing of other attributes, e.g.:
        if attr == 'group_id':
            try:
                val = int(val)
            except ValueError:
                # maybe add a log entry... then
                val = None

        setattr(m.meta, attr, val)  # <-- This *MUST* be called for every attribute in RECOGNIZED_MEMBER_FIELDS

[hbushouse]

Looks good. I'm just wondering if we need a regression test given that this is not run in the pipeline.

That would be good, because by default it's not used in normal processing, so we'd have no idea if something in it went bad. @mcara Do you have test data that you used when developing the code updates, which could be turned into a new regression test?

[hbushouse]

The CI failures appear to be unrelated and caused by some test data files not being included in the PR branch (such that tests can't find the inputs/outputs they need).

[hbushouse]

I'm going to merge this now, so that we can have it included in our next B9.0 release candidate. It's not necessary for use in the operations pipeline, but that way it'll be available to users via the 9.0 release.

[hbushouse]

A regression test can be added later in a separate PR.

[mcara]

It is somewhat difficult to have this as a unit test. Maybe some parts of the model container can be tested as a unit test.