Rework strict_config

[thomaswilliamsastro]

This PR allows for more granular control over what arrays to include in a combination, and deprecates the old "strict_config" flag.

The config_definitions now has a 'requires' keyword for interferometric configs, which works by using a pipe ("|") like an OR statement. So for instance, you can now set up a 12m config like this:

interf_config   	  12m       {'array_tags':['12m_1','12m_2']}
interf_config   	  12m       {'requires':['12m_1|12m_2']}

which will read as you require at least one of 12m_1 or 12m_2. If a particular array isn't included anywhere in the 'requires' list, it's assumed to be required to be present. So in a more complicated case:

interf_config   	  12m+7m    {'array_tags':['12m_1','12m_2','7m']}
interf_config   	  12m+7m    {'requires':['12m_1|12m_2']}

requires at least one of 12m_1 or 12m_2, AND also requires 7m. This means that the old, default strict_config=True behaviour is maintained just by not including this new 'requires' line, and strict_config=False can be maintained by (e.g. for 12m+7m):

interf_config   	  12m+7m    {'requires':['12m_1|12m_2|7m']}

Trying to set strict_config to True or False in the VisHandler now raises a DeprecationWarning, and this new 'requires' behaviour is documented both in the KeyHandler and VisHandler.

Importantly, this enables us to image datasets where we might have 2 12m configs in some cases but not others, but you don't want to allow it to image everything if e.g. the 7m is missing (the use case here is SSCALES). I've tested this on a number of cases and seems to be working as expected, though it would be good to get some eyes on just to make sure this is robust.

• Rework strict_config in the KeyHandler and VisHandler
• Update scripts to remove strict_config calls
• Update CHANGES.rst

[codecov]

Codecov Report

❌ Patch coverage is 3.70370% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 6.27%. Comparing base (9997b7b) to head (470e518).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
phangsPipeline/handlerKeys.py	3.84%	50 Missing ⚠️
phangsPipeline/handlerVis.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #329   +/-   ##
======================================
  Coverage    6.27%   6.27%           
======================================
  Files          38      38           
  Lines       15229   15250   +21     
  Branches     3653    3661    +8     
======================================
+ Hits          955     957    +2     
- Misses      14260   14279   +19     
  Partials       14      14           

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

[e-koch]

@thomaswilliamsastro is this ready for review now?

[thomaswilliamsastro]

@e-koch yep!

[e-koch]

Looks good!

Can you add a unit test for requires? Or we can make a separate PR to setup a broader keys unit test suite: https://github.com/PhangsTeam/phangs_imaging_scripts/blob/master/phangsPipelineTests/test_handlerKeys.py?

[thomaswilliamsastro]

@e-koch I think I'd prefer to make it a separate PR to port over unit tests for the handlerKeys and add this in

[akleroy]

I think we should discuss this - this is not something that we've discussed (to the contrary the other suggestion was shot down) and I am not sure I agree that we want less-specific file names or to force the multi-scale choices to be automated?

I'm happy to be overruled if we discuss but it did not seem to me like there was actual consensus on this.

I don't think it should merge before we discuss.

[akleroy]

I definitely agree that we weren't using the old "strict config" and it was always left True to my knowledge so agree that cleaning that part up is good - but we don't appear to agree as a group on how to deal with hybrid configs in an infrastructure way.

[thomaswilliamsastro]

@akleroy This shouldn't tie anything up into multiscale clean scale choices. The idea here was just to make the "strict config" more clear and give more control if you didn't want the combos to necessarily have to include everything. The use case specifically here was to make the NGC300 and NGC7793 12m+7m possible to actually image, but if we do want to go the route of more specific filenames then I still think this is useful for the future for folks who just want to lump all their 12m data together without worrying about whether ALMA calls it extended or compact. But happy to discuss further in an ADR

[akleroy]

But it will affect the multiscale because it's setting the precedent / intention that we mix and match specific array configs into the same hybrid config name, so that the only option will be to use the beam-size scaling right because we can no longer be sure of what's actually in a 12m+7m file?

This all may be fine but it's something that it did not seem like there was consensus on this.

[thomaswilliamsastro]

@akleroy I'm not sure this necessarily means we'd have to bake in clean scales, but agree it means filenames can be less descriptive. I think this functionality is needed whichever case we land on, though.

• If we stick with no configs in the names (12m+7m) then we need this for NGC300 since it only has the one config
• If we want to go specific and have different configs for each galaxy, then we need this for NGC7793, since the archival field has only one config (and I think that's a different one to the other observations). So in that case we still need to relax which configs are included so we can get a full mosaic

Beyond that, I think having this for archive dives is useful. The eCMZ folk seemed pretty keen on having something like this