Skip to content

ParSet Refactor#2097

Open
kbwestfall wants to merge 77 commits into
pypeit:developfrom
kbwestfall:parset_refactor
Open

ParSet Refactor#2097
kbwestfall wants to merge 77 commits into
pypeit:developfrom
kbwestfall:parset_refactor

Conversation

@kbwestfall

@kbwestfall kbwestfall commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator

This is a major refactor of the ParSet base class.

Many files are touched but the primary changes are in pypeit/par/parset.py and pypeit/par/pypeitpar.py. Branches that include changes to these files will likely have significant conflicts. I apologize for the headaches, and I'm happy to help with merges. Beyond those two files, the refactor is virtually invisible to the rest of the code, only requiring some changes to imports and some auto-generated doc scripts.

The main accomplishment of this PR is that it dramatically cleans up and streamlines the construction of ParSet subclasses. Instead of defining the parameters based on the combination of the call sequence of the __init__ function and the subsequent body of the function, everything is defined as part of a dictionary class attribute called parameters. This allows the refactored ParSet class to handle everything except the definition of those parameters and any validation methods. I.e., none of the current ParSet subclasses require a bespoke __init__ function; they all can make do with the base class method.

I used AI to help with this, and a script that compared the old and new versions of the ParSet subclasses (the parameters, types, options, and descriptions). After the initial refactor, in which I explicitly made both the old and new subclasses identical, I edited all of the descriptions. Most of this was to ensure a fixed line length, but I also edited the content of some.

All the unit tests pass, and I'll set the dev-suite going now.

@kbwestfall

Copy link
Copy Markdown
Collaborator Author

Getting some test failures:

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  329 passed, 448 warnings in 314.48s (0:05:14) ---
--- PYTEST UNIT TESTS FAILED  1 failed, 158 passed, 1438 warnings in 837.21s (0:13:57) ---
--- PYTEST VET TESTS FAILED  3 failed, 71 passed, 1346 warnings in 5075.07s (1:24:35) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 2/292 TESTS  ---
Failed tests:
    gemini_gmos/GS_HAM_MULTI_R400_700 pypeit
    ldt_deveny/DV7 pypeit
Skipped tests:
Total disk usage: 358.994 GiB
Testing Started at 2026-03-31T19:55:32.024620
Testing Completed at 2026-04-01T10:01:25.810344
Total Time: 14:05:53.785724

The unit test failure is the issue with the number of LDT/DeVeny DV1 files, but the rest are new. I'll work on these and run the tests again.

@kbwestfall

Copy link
Copy Markdown
Collaborator Author

I ran into a stumbling block with this implementation.

TLDR, path parameters are now a callable function (Path.cwd) by default. Parameter subsets, like ReduxPar and Collate1DPar, leave the parameter as the function after instantiated, but the superset, PypeItPar, fills out the path (into a Path object) as part of instantiation as part of the validate function. If you ever instantiate one of the subsets, you need to run the fill_callable function or use the path parameter as a function instead of a Path object.

The headache boils down to the test_wavelengths.py::test_redoslits_kastr vet test. The test uses the completed calibration files from a shane_kast_red reduction, masks one of its slits, then uses the redo_slits parameter to fill it back in again. The issue was that the test actually changes directories before executing run_pypeit. By defining the default paths for, e.g., the redux_path parameter in the ReduxPar ParSet as a class attribute, the path is defined when pypeit is imported, not when the parameter set is instantiated. This is essentially not a problem for most use cases of pypeit because none of the scripts themselves ever actually change directories within the file system during the execution of the script. I.e., the path where any script is executed remains the same throughout the execution. But, for this test, this meant that the default path used was not right because it did not reflect the real-time current working directory.

The fix I have implemented means that the redux_path parameter of the ReduxPar parameter set is now a callable function. This allows the default path to be abstract until it is needed. In terms of the PypeItPar parameter superset, this change is effectively invisible. I.e.:

>>> from pypeit.par import pypeitpar
>>> p = pypeitpar.PypeItPar()
>>> p['rdx']['redux_path']
PosixPath('/Users/westfall/Work/packages/PypeIt-development-suite')

This is because I've added a recursive function that fills in the callable values as part of the validate method. However, this is not true for the parameter subsets:

>>> rdxp = pypeitpar.ReduxPar()
>>> rdxp['redux_path']
<bound method PathBase.cwd of <class 'pathlib._local.Path'>>
>>> rdxp.fill_callable()
>>> rdxp['redux_path']
PosixPath('/Users/westfall/Work/packages/PypeIt-development-suite')

This is admittedly a bit janky, but I think the benefits of the refactored ParSet outweigh this nuance. Happy to hear other opinions though.

As a by-product of this, I also went through the uses of redux_path in ReduxPar and outdir in Collate1DPar to ensure that they can be either strings or Path objects.

@kbwestfall

Copy link
Copy Markdown
Collaborator Author

Tests pass

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  331 passed, 448 warnings in 258.56s (0:04:18) ---
--- PYTEST UNIT TESTS FAILED  1 failed, 158 passed, 1438 warnings in 768.99s (0:12:48) ---
--- PYTEST VET TESTS PASSED  74 passed, 1662 warnings in 4563.41s (1:16:03) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 292/292 TESTS  ---
Total disk usage: 364.262 GiB
Testing Started at 2026-04-02T19:48:39.797438
Testing Completed at 2026-04-03T07:16:23.683146
Total Time: 11:27:43.885708

The unit test failure is because of the number of ldt_deveny/DV1 files.

@profxj profxj requested review from badpandabear and profxj April 6, 2026 21:21

@profxj profxj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked carefully at parset.py and bit at pypeitpar.py

Would be good to convince ourselves all the default values
stayed the same before/after. Am betting Kyle has already
checked that..

Am approving

Comment thread pypeit/par/parset.py
Comment thread pypeit/par/parset.py Outdated
Comment thread pypeit/par/parset.py
Comment thread pypeit/par/pypeitpar.py
Comment thread pypeit/par/pypeitpar.py

@badpandabear badpandabear left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My main concern is about what appears to be unnecessary changes to the way PypeItPar is imported, but maybe I'm missing something and there's a good reason for it. I also had a comment about collate_1d's command line usage which is really my fault.

Comment thread doc/help/pypeit_collate_1d.rst Outdated
Comment thread pypeit/scripts/collate_1d.py Outdated
Comment thread pypeit/par/__init__.py
@@ -1,2 +0,0 @@

from pypeit.par.pypeitpar import PypeItPar

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change caused a lot of the changes in this PR. Why was it done? Was it considered a circular dependency? Because I believe from .pypeitpar import PypeItPar would work without having to change a lot of code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I appreciate that this is a bit obnoxious. I've soured on this kind of convenience import in __init__.py files. It does create a "cycle" that pydeps identified, and I was also trying to unify how we import pypeitpar across the code base. If you have really strong objection to this, I can revert it.

Comment thread pypeit/scripts/chk_for_calibs.py
@tbowers7 tbowers7 added the Refactor Refactoring code for performance or maintenance improvement without adding significant new features. label Apr 15, 2026
@kbwestfall

Copy link
Copy Markdown
Collaborator Author

This has two approvals, but don't merge it yet. I'm re-running the dev-suite, and I think we should try to get current PRs that make changes to parameters in before this one.

@kbwestfall

Copy link
Copy Markdown
Collaborator Author

Tests still pass.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  331 passed, 449 warnings in 447.06s (0:07:27) ---
--- PYTEST UNIT TESTS FAILED  1 failed, 158 passed, 1438 warnings in 1249.97s (0:20:49) ---
--- PYTEST VET TESTS PASSED  74 passed, 1662 warnings in 6552.66s (1:49:12) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 292/292 TESTS  ---
Total disk usage: 298.957 GiB
Testing Started at 2026-04-28T23:23:03.045140
Testing Completed at 2026-04-29T19:37:27.465263
Total Time: 20:14:24.420123

@tbowers7 tbowers7 modified the milestones: v2.1.0, v2.2.0 May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactoring code for performance or maintenance improvement without adding significant new features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants