Remove unused code and expand unit tests

[sbailey]

This maintenance PR removes unused code and expands unit test coverage of remaining code. For the moment the "removed" code was actually moved to a deprecated/ directory so that we can easily get it back if I moved too much without having to swap branches, but the intension is to delete the deprecated/ directory once we are sure this is ok.

Removed modules and related scripts and tests:

• desispec.pipeline (the pre-operations one that used a database; replaced by desispec.workflow in current use)
• desispec.quicklook (superseded by separate repo nightwatch; qproc stuff is still kept)
• previous generation qa (never used in prod; replaced by Anand's QA)
• test/integration_test and test/old_integration_test (based on sims, not working and not used now)

Expanded/Updated tests for badcolumn, binscripts, bootcalib, efftime, fiberflat, image_model, workflow.timing, xytraceset.

New features to make some of the tests easier / faster:

• image.Image constructor sets meta['CAMERA'] if camera is known
• xytraceset.XYTraceSet supports slicing to subset of fibers

With these updates, the coverage when run at NERSC increases from 27% to 47%; see https://data.desi.lbl.gov/desi/users/sjbailey/temp/htmlcov_pr/ . That's still low coverage but this branch is getting to be a lot of changes, so I suggest that we review this now and then further expand coverage in future PRs.

[coveralls]

coverage: 39.449% (+9.5%) from 29.988%
when pulling 3f85102 on deprecated
into 55936a5 on main.

[weaverba137]

@sbailey, we may need to check the coverage configuration. The tests themselves are being included in the code to be tested and that is significantly decreasing the coverage.

[sbailey]

The fiberflat gradient test is consistently failing on some but not all github configurations, and passing at NERSC and on my laptop. This smells like individual tests having side effects that impact the results if run in a different order, but I haven't figured out what the core issue is.

[sbailey]

@sbailey, we may need to check the coverage configuration. The tests themselves are being included in the code to be tested and that is significantly decreasing the coverage.

Curious. What I originally posted was run with pytest --cov=desispec --cov-report=html py/desispec/test, and that included the tests as a positive contribution to the coverage since they had ~100% coverage each. But then I noticed that the .coveragerc file specifically omits py/desispec/tests/ and I wondered if --cov=desispec was overriding that so I reran with pytest --cov-report-html py/desispec/test, and that still included the tests in the output report but now with 0% coverage, pulling the total coverage down. In some sense this is just cosmetic, but @weaverba137 if you know what we should be going instead (both on github and when running the more complete set of tests at NERSC), please update that.

And, after adding debug printouts trying to figure out why the fiberflat tests are failing on github, they are working again. I'll poke at that a bit more, but if they continue to pass I'll accept that. :)

[weaverba137]

In some sense this is just cosmetic

I slightly disagree with that. The number passed to Coveralls is what people see on GitHub. I know there are reasons that number may be different from the number at NERSC. Effectively, nobody but you ever sees that number.

Conceptually, I think passing py/desispec/test to pytest is short-circuiting its natural test-discovery process, so we need to start with configuring pytest to skip deprecated, not force it to read tests from one directory.

[sbailey]

Due to PR #2424 adding desispec.scripts.purge_tilenight, there was a merge conflict with this branch in api.rst due to that added entry adjacent to other deprecated modules removed by this PR. I resovled that by retaining it in this branch, which I think is going to cause the doc tests to fail because purge_tilenight doesn't exist in this branch. But if I had removed it in the merge conflict, it would fail after merging. i.e. let's not worry about the doc tests failing here, and do any additional patching if needed after merging.

[akremin]

I think the following should also be deprecated. The first two I already pointed out privately:
bin/desi_quicklook
bin/wrap_desi_night.sh
bin/desi_pipe
bin/desi_pipe_exec
bin/desi_pipe_exec_mpi
bin/desi_pipe_status
bin/desi_night
bin/desi_daily_proc_manager

[sbailey]

Thanks for identifying those additional deprecated scripts. I moved those and also moved py/desispec/scripts/daily_processing.py which was wrapped by bin/desi_proc_manager but not used elsewhere.

[akremin]

Thank you for this needed spring cleaning. 112 is more files than I expected this to touch, but everything looks appropriate.

I won't merge until you've sorted out the test_gradient_correction test that is still failing:
FAILED py/desispec/test/test_fiberflat.py::TestFiberFlatObject::test_gradient_correction - RuntimeError: gradient center-to-edge difference larger than 20.0% !

Everything else looks good. I didn't do an in-depth analysis of all the unit tests as there were too any of them, but from going through them they generally look good.

I went through all of desispec and identified several other files to deprecate, which I posted separately and have already been addressed (thank you). I didn't find any outstanding issues of dependencies on deprecated code, so hopefully we are covered there.

[weaverba137]

Now we're showing a coverage increase, so I think that configuration is all set.

[sbailey]

In some sense this is just cosmetic

I slightly disagree with that. The number passed to Coveralls is what people see on GitHub. I know there are reasons that number may be different from the number at NERSC. Effectively, nobody but you ever sees that number.

What I meant by "cosmetic" is that a) what really matters is what our coverage is, not what some badge says; and b) regardless of whether the tests themselves are included in the stats or not and whether we are running at NERSC or not, our coverage is low and the difference of the tests doesn't move us from a respectable coverage to an embarrassing level anyway. It's low in either case.

That being said, if there is a cleaner way to configure that, great. I don't understand why the .coveragerc file isn't already excluding desispec.test from the stats.

UPDATE: while I was typing that, @weaverba137 added a commit moving pytest and coverage info to setup.cfg. Nevermind.

Conceptually, I think passing py/desispec/test to pytest is short-circuiting its natural test-discovery process, so we need to start with configuring pytest to skip deprecated, not force it to read tests from one directory.

That is a temporary hack to prevent it from discovering tests in the deprecated/ directory which we plan to remove after converging on what is supposed to be deprecated. I could have done this via a pytest.ini file instead which I admit would have been a bit cleaner.

I see that while I was typing this, @akremin approved the changes, so now before merging I will remove the deprecated/ directory and restore the github actions pytests to fully auto-discover. The fiberflat tests stopped failing without me taking specific action and now I can't get them to fail again, so I'm going to move on unless/until the failures re-appear.

[weaverba137]

Please make sure you have picked up my latest change. I would not recommend further changes to the GiHub Actions configuration.