Unskip and refactor effective area and temperature response IDL comparison tests

[wtbarnes]

This PR refactors the tests for the effective area and temperature response calculations and does some very minor refactoring of the code itself. Below is a brief summary:

• The IDL comparison tests for the effective area calculation were being skipped somehow. This unskips them.
• Remove the pytest-allclose extension because it does not seem to be actively maintained and its utility seems limited.
• Compute the filter and CCD contamination curves on a dense wavelength grid to avoid inaccuracies in the interpolation from the original data near the discontinuous features in these curves
• General refactoring and simplification of the setup for the effective area and temperature response tests.
• Removed a channel test that was unneeded

Notably, this PR has revealed two things about the IDL comparison tests:

• In the IDL calculation of the contamination curves, quadratic interpolation is used which leads to ringing/overshoots near the discontinuous features in those curves. As such, the differences with the (correct) xrtpy implementation here will always be significant near these features. I've marked all of these tests as xfail for now until the IDL code is fixed and the IDL test files are regenerated.
• In the IDL calculation of the temperature response curves, it's clear that the right gain value is being pulled from the original data file rather than using the more correct value (see below). As such, I've set the relatively tolerance for these comparison tests to be quite high (5%). I checked and if the gains are set to be the same (from the file), the differences are less than 1%. This should be fixed in the IDL code and the test data regenerated.

To be clear, I've not updated the IDL test data here and have instead left that to a future PR. I've created issues for both test data sets to ensure that we don't lose track of this.

[namurphy]

+1 on dropping pytest-allclose, in particular if it's no longer being maintained.

Since the tests appear to have been failing, an alternative would be to mark them with @pytest.mark.xfail instead to indicate that.

[wtbarnes]

Ok it turns out that the IDL results are in fact not correct and that what we are doing on the Python side is correct. The fundamental issue is that in the IDL code, the wavelength is being interpolated to the contamination curves using a quadratic interpolation scheme (from make_xrt_ea.pro)

ctrans2 = interpol(contam_in[ichn].trans[0:clen[ichn]-1], $
                         cwave1, wave2, /quad              )

This is problematic near the sharp edges in the contamination curve because it leads to the interpolation scheme slightly overshooting the original contamination data array. Below is a screenshot zoomed into one of these features. The red is the original data array of the filter contamination data. The black is the interpolated curve.

These only happen at a few places in the curves, but they lead to differences of over 10% compared to doing the interpolation linearly with a sufficiently dense grid.

[jslavin]

Very interesting. So we need to fix the IDL code, maybe and produce results from that for use in the tests? It sorta violates the spirit of making our results consistent with SolarSoft, but then again when SolarSoft is wrong it's best not to propogate that error.

[wtbarnes]

Yes, I would say the IDL code should be fixed to to not use quadratic interpolation. Is there an easy script to reproduce all of the IDL test data? I'm happy to do that here if so.

I'm not sure how one goes about actually pushing an IDL fix upstream.

[jslavin]

Yeah, it's not clear. I don't know about contributing fixes to SolarSoft. I tried to email Sam Freeland a while back about a bad link on a SolarSoft page (since his email was listed on the page) but the email bounced. I'd say just edit your local copy and produce the data. I think maybe Joy has a script to do this.

[joyvelasquez]

Hi @nabobalis,
Sorry for the delay in reviewing this PR. It has been on our list of things to do. We're holding our XRTpy meeting tomorrow ( Oct 18) as a group, and reviewing this PR is a main topic on our agenda. We plan to discuss it in detail with the team and will provide feedback after that. This goes for other pending issues as well.

[nabobalis]

Ah I see, very sorry for the premature merge!

Will and I can fix any mistakes within this PR after that meeting.

[joyvelasquez]

Haha, no worries!
I’ve been a bit slow lately since I haven’t received a replacement laptop (die after our meeting at the CfA), and my current one is too old to update, so I’m limited in what I can do. However, we greatly appreciate your support and dedication to XRTpy!

[joyvelasquez]

Update: We held our XRTpy group meeting and discussed the next steps. We're planning to update the IDL function that calculates the effective area and temperature response. Afterward, we'll create new data products to test against XRTpy results. Once the results match, we'll proceed with updating the XRT IDL routine.

[joyvelasquez]

Hey @wtbarnes & @nabobalis ,
I'm currently working on analyzing a new set of IDL-updated effective area data files. I've applied an update that switches the interpolation method to linear, and I’d like your feedback on the relative tolerance value. The current setting in this PR is at 1e-6, but I'm considering if 1e-3 might be more appropriate.
Do you have any thoughts on this adjustment?
@jslavin , please share your thoughts a well.
Thanks in advance for your input!

[nabobalis]

Does it pass with 1e-6 or does it need to be lower?

[joyvelasquez]

I've tested several relative tolerance values. At 1e-6, all tests fail, while at 1e-5, some pass, and others show fewer failing values. As I reduce the tolerance further, most tests pass, though a few still have isolated failing values. I'm working on a straightforward analysis and aim to share the latest findings today or early next week, including plots that highlight where these issues are most prominent and which filters are most affected.

Next Tuesday, I'll present this idea and preliminary results to the XRT team during our calibration meeting. I hope to gather more insights on the original IDL script used for this analysis and how XRTpy's approach compares with IDL.

[nabobalis]

Can you produce IDL plots like Will did of the fitting with the linear option?

Maybe something odd is happening with IDL interpol?

[jslavin]

I wonder if part of the problem is that the test interpolates the IDL result (as printed to files) with the XRTpy result. The XRTpy effective areas are defined over a much finer wavelength grid. I think it would make more sense to interpolate the XRTpy result to the IDL wavelength grid for the comparison. Jon

…

On Fri, Oct 25, 2024 at 2:49 PM Nabil Freij ***@***.***> wrote: Can you produce IDL plots like Will did of the fitting with the linear option? Maybe something odd is happening with IDL interpol? — Reply to this email directly, view it on GitHub <#284 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE5OYYYTSHLAWJLTB45GQDZ5KHE7AVCNFSM6AAAAABNTQBQXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGU4DKMJYHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>