Fix SWAPI integration tests - #175
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes SWAPI integration tests by aligning subprocess data paths and correcting dependency fixtures.
Changes:
- Passes the staged data directory to processor subprocesses.
- Adds the missing L3b dependency manifest.
- Pins SWAPI and MAG fixtures to correct revisions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/integration/test_swapi_processor_integration.py |
Aligns subprocess data paths and revised L2 fixture lookup. |
tests/integration/test_data/swapi/imap_swapi_l3b_20260101_v001.json |
Adds the missing L3b dependencies. |
tests/integration/test_data/swapi/imap_swapi_l3a_pui-he_20260101_v001.json |
Pins revised SWAPI science data. |
tests/integration/test_data/swapi/imap_swapi_l3a_proton-sw_20260101_v001.json |
Pins revised SWAPI science data. |
tests/integration/test_data/swapi/imap_swapi_l3a_alpha-sw_20260101_v001.json |
Pins revised SWAPI and MAG science data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jtniehof
left a comment
There was a problem hiding this comment.
Minor comments, except that test_alpha_sw_with_production_data still fails OMM:
try:
> numpy.testing.assert_allclose(
actual_value, expected_values[key], rtol=rtol, atol=atol, err_msg=key
)
E AssertionError:
E Not equal to tolerance rtol=0.001, atol=0
E alpha_sw_density
E Mismatched elements: 1 / 1 (100%)
E Max absolute difference among violations: 0.00503448
E Max relative difference among violations: 0.03190463
E ACTUAL: array(0.162832)
E DESIRED: array(0.157798)
/home/jnik/work/scm/imap_L3_processing/tests/integration/test_swapi_processor_integration.py:194: AssertionError
Does this look like something you've come across in creating this fix (in which case maybe there's some lurking "wrong file" thing), or do I need to go chasing "from scratch"?
Yes, I did have a couple of similar failures! I had no good reason for the particular version choice of the science/ancillary files other than "it works for me". |
Hmmm, so does this mean somehow I'm pulling the wrong version, or that there's something incomplete about the fix, or I'm holding it wrong? Let me try rerunning with my data directory removed, so I don't have stale files. EDIT: |
|
I can get the alpha test closer by using Is this working on your machine right now? I'm wondering if this is something where we updated the alpha algorithm without updating the integration test (since it basically wasn't working). |
I just ran these commands (please be careful with the git switch 171-172-174-fix-swapi-integration-testing
git clean -nd
git clean -ndX
rm -rf .git/lfs/objects
git lfs pull
uv run --with pytest pytest tests/integration/test_swapi_processor_integration.pygives me Since we made real changes in the names of the dependency files, I'm not convinced this is a code issue and not simply an input error. But I would note that I've encountered several other projects with integration tests success being OS- and Python-version-dependent. This can be specially true for tests that focus on computing relative errors like this one. |
There was a problem hiding this comment.
To get the density to match OMM, this rtol needs to be 0.03 or so. I really dislike this, but I think it's the best way forward.
There was a problem hiding this comment.
I bumped the dependency to imap_mag_l2_norm-rtn_20260101_v001.0005.cdf, which caused the tests to fail OOM. I then bumped some of the rtol to 0.02 and they passed again. Could you check if the combination of dependency and tolerances currently in the repo works for you as well?
Thanks. I did this cleanup and it still didn't work for me. The only other thing I can think of is that my imap_processing was a few commits behind, so I'll try one more time, but I think we should just go ahead and update the mag and relax the tolerance. I'm curious to see what changing the mag file version does to your results. |
e9f0b9a to
58ce6cb
Compare
…ed as a DependencyFilePath
58ce6cb to
74bea71
Compare
Change Summary
Closes #171, closes #172, closes #174.
Overview
imap_l3_data_processor.pynot inheriting the parent test'sDATA_DIR, causing it to look for staged inputs in the wrong place.imap_swapi_l3b_20260101_v001.jsondependency fixture, which was never checked.Testing
Run:
passes. Also verified the fix is robust to an arbitrary ambient
IMAP_DATA_DIR(the original bug in #171):Both pass identically on my machine.