JP-3810 followup: small clean-up tasks in AMI module

[emolter]

Follow-up of JP-3810

Closes #9437

This PR addresses a few small follow-up items from the larger AMI clean-up PR:

• Remove TODO statement in hextransformee, replacing it with a comment explaining that the behavior in question is intended
• Fix a bug in a unit test to matrix_dft
• Fix Ami/analyticnrm2 psf function has error-prone logic #9437 by just removing the "fringeonly" option, which was already unusable on main prior to the PR for JP-3810.
• Fix "circ" and "circonly" options to analyticnrm2.psf erroring out

Tasks

• If you have a specific reviewer in mind, tag them.
• add a build milestone, i.e. Build 12.0 (use the latest build if not sure)
• Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
  • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job (click here for instructions)
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

[emolter]

The problem was that the argmax call on dft_offset is multi-valued if offset is an integer. Prior to these changes, there were 4 pixels with equal value. It seems that numpy 2 picks a different one of those 4 pixels to use than numpy 1, which led to this error. The test was fixed by simply using an offset that does not contain integer values. There was nothing wrong with the matrix DFT code itself, the test was just badly written.

[emolter]

regression tests https://github.com/spacetelescope/RegressionTests/actions/runs/14891477971

[tapastro]

Related to the sonarscan PR: is there any worry that the psf_offset is implemented in the asf_hex and asffringe functions, but the asf function had it dropped? I don't know enough about the background of the algorithm to tell if this would produce incorrect results, e.g. in the results from the psf function for the asf_fringe values provided to the asf_2d calculation.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.90%. Comparing base (9c837cd) to head (43bbbf0).
⚠️ Report is 649 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9441      +/-   ##
==========================================
+ Coverage   77.86%   77.90%   +0.03%     
==========================================
  Files         362      362              
  Lines       36335    36340       +5     
==========================================
+ Hits        28294    28310      +16     
+ Misses       8041     8030      -11     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[emolter]

@tapastro Thanks for bringing this up. I'm having a side-conversation with @rcooper295 and @anand0xff that may make this irrelevant - they might want to just remove the option to provide an offset at all

[emolter]

is there any worry that the psf_offset is implemented in the asf_hex and asffringe functions, but the asf function had it dropped? I don't know enough about the background of the algorithm to tell if this would produce incorrect results, e.g. in the results from the psf function for the asf_fringe values provided to the asf_2d calculation.

The asf() function did not use the offset even prior to JP-3810. It looks to me like the math for the "circ" shape was separated into the asf() and asffringe() parts, and then for "circonly" the asffringe() part is not included (which means that the offset will be ignored and the psf will always be centered). Not sure if this is correct, but what I've done reproduces the algorithm as it was.

But this leaves a few questions here for @rcooper295 or @anand0xff:

• Is it the correct behavior that asf() doesn't support an offset? It looks like the function jinc(), which asf() calls, has optional offx and offy parameters that aren't used. asf() also differs from asf_hex() and asffringe() in that it doesn't have the line im_ctr = image_center(fov, oversample, psf_offset). Should it?
• Is it ok that I just re-named the variable to asf_2d and allowed it to pass through for the "fringeonly" option? Put another way, does the asffringe() function, when called standalone, produce a valid PSF?
• Do we even want to deal with validating any of this, or should we instead make a separate ticket and PR to remove the psf_offset parameter in the spec of AmiAnalyzeStep? See discussion here

[rcooper295]

• Is it the correct behavior that asf() doesn't support an offset? It looks like the function jinc(), which asf() calls, has optional offx and offy parameters that aren't used. asf() also differs from asf_hex() and asffringe() in that it doesn't have the line im_ctr = image_center(fov, oversample, psf_offset). Should it?

I think it should; unless there's something I'm missing I don't see why the circular case shouldn't support an offset the way the hex case does. Though currently I'm getting the error that affine2d is an unexpected argument being passed to jinc() in the asf() calls (in the "circ" and "circonly" cases; "hex" and "hexonly" seem to be working).

• Is it ok that I just re-named the variable to asf_2d and allowed it to pass through for the "fringeonly" option? Put another way, does the asffringe() function, when called standalone, produce a valid PSF?

I think that should be fine; I see that it previously failed since asfd wasn't defined in that case. The "psf" produced with that change looks reasonable to me.

• Do we even want to deal with validating any of this, or should we instead make a separate ticket and PR to remove the psf_offset parameter in the spec of AmiAnalyzeStep?

I don't think we should remove the psf_offset param, but I'm not sure I'm following your reasoning -- is it not actually doing what we seem to think it's doing?

@anand0xff does this seem ok?

[emolter]

Thanks very much for these responses. I will fix the "circ" and "circonly" cases and see if they look reasonable... the expectation that it yields an Airy ring because it's just the Fourier transform of the circular aperture, is that correct?
I'll also add some unit tests here - it looks like I didn't do a good job getting coverage of this in the original PR.

I don't think we should remove the psf_offset param, but I'm not sure I'm following your reasoning -- is it not actually doing what we seem to think it's doing?

This was related to the same conversation we were having before about offsets in the matrix DFT. From my reading of the code, these two offsets are related, i.e. if we wanted to drop support for a user-specified psf_offset as a parameter to AmiAnalyzeStep we could remove both. But it sounds like that's NOT the direction you want to go, so it's irrelevant.

[emolter]

ok @rcooper295 the psf() function should now at least run in the "circ" and "circonly" cases, but I'm a bit concerned that it's giving us the correct behavior. Would you please validate this and let me know what you'd like to see changed?

• "circonly" does not respect the PSF offset, and has its maximum at (0,0) instead of (4,4) like the rest of these options. (See the unit test failures.) Should this be fixed somehow?
• To fix the above, I tried simply allowing psf_offset to pass through asf() and into the offx, offy parameters of the function jinc() in order to try to fix this, but it didn't seem to have any noticeable effect. Plus, this doesn't make a lot of sense c.f. the circ case, where the psf_offset is passed into asffringe() but not into asf(). Are the parameters offx, offy actually doing something useful inside jinc()? If so, should they be accessible via psf_offset and how? If not, should they just be removed?
• It seems suspicious to me that asf_hex() can accept an Affine2d object but asf() cannot. I don't understand this part of the code particularly well, but it was my understanding that Affine2d represents some kind of geometric distortion?

Please let me know how you'd like to see this changed. Are the "circ" and "circonly" options even necessary to continue supporting?

[rcooper295]

@emolter I see what you mean about the "circ" and "circonly" cases being at 0,0 rather than the center of the array, and the offset not having an effect when passed directly to jinc(). But by passing psf_offset to asf() and calculating im_ctr and using that as the offx and offy inputs to jinc(), it seems to do what I'd expect. Locally my asf() function now reads:

    pitch = detpixel / float(oversample)

    im_ctr = image_center(fov, oversample, psf_offset)

    return np.fromfunction(
        jinc,
        (oversample * fov, oversample * fov),
        d=d,
        lam=lam,
        pitch=pitch,
        offx=im_ctr[0],
        offy=im_ctr[1]
    )

Passing the offset to both asf() and asf_fringe() in the "circ" case also seems to work ok (places the fringes and the primary beam at the same offset).
I believe the circ/circonly options are included to make debugging easier, since it's easier to manipulate the circular transform than the hexagonal one, but they're not strictly necessary for the function of the code. That may be why they didn't bother including the affine distortion argument (though it seems like that could similarly be helpful for a simple "sanity check"). I don't think we need to worry about adding it in though.

[emolter]

Thanks very much @rcooper295 , with your suggested changes the output makes sense to me, too. Going to run our testing suite and then mark this ready for review soon. Would you mind giving it a quick look-over to see if the changes here make sense to you?

[emolter]

regtest rerun is here https://github.com/spacetelescope/RegressionTests/actions/runs/14891477971

[rcooper295]

These changes to clean up and fix some of the accessory functions for AMI modeling look good to me.