Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spx io refactor #1082

Merged
merged 25 commits into from Feb 11, 2021
Merged

Spx io refactor #1082

merged 25 commits into from Feb 11, 2021

Conversation

marcelo-alvarez
Copy link
Contributor

(this pull request is identical to and meant to accompany desihub/specex#36)

A major change to how specex is called by desispec, using pybind11.

Calculations performed on the same day using (i.e. with specex.py as called by desi_compute_psf_mpi)
https://github.com/desihub/desispec/tree/spx_io_refactor
and (with directions for compiling the python shared object module in subdirectory specex_pybind)
https://github.com/desihub/specex/tree/io_refactor
should produce an identical merged psf fit file to that using
https://github.com/desihub/desispec
and
https://github.com/desihub/specex

Please note this pull request is additionally dependent on branch of https://github.com/desihub/specex, as reflected in the URLs above.

@marcelo-alvarez marcelo-alvarez marked this pull request as draft January 5, 2021 22:26
This was referenced Jan 5, 2021
@coveralls
Copy link

coveralls commented Feb 1, 2021

Coverage Status

Coverage increased (+0.3%) to 29.629% when pulling 29bf94c on spx_io_refactor into 83a6001 on master.

@marcelo-alvarez marcelo-alvarez marked this pull request as ready for review February 1, 2021 22:36
@julienguy
Copy link
Contributor

Successfully tested along with specex branch io_refactor.
Results as same PSF as Mt Blanc for the same inputs.
Ready to merge for me.

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

We're sorting out some pybind11 cleanup and installation procedures on the specex side, but once that is resolved and merged, ok to merge this too.

@sbailey
Copy link
Contributor

sbailey commented Feb 3, 2021

@weaverba137 do you know how to update the coverage tests to be informative but not flag the PR as failing tests due to a 0.2% drop in coverage?

OK to merge once desihub/specex#36 is merged.

@weaverba137
Copy link
Member

For a long time, we weren't getting any advice at all about coverage because it had been disabled entirely. I re-enabled in what I thought was an advice-only state. However, it appears that not specifying a decrease threshold actually corresponds to any decrease being flagged as an error, rather than simply sending an informational message.

So, what thresholds do we set? There are two that need to be specified:

  • What total level of coverage is considered acceptable? If I were starting a project from scratch, I would not consider 29% acceptable, but we'll have to grandfather desispec.
  • What decrease of coverage is considered tolerable? The value is change in total coverage, not change in fractional coverage. The old coverage was 29.3% and now it's 29.1%, so that's -0.2%. There's no dividing percentages by percentages going on. Note that for a large package such as desispec, even a small decrease in coverage could represent a large number of lines that are not or no longer tested.

@sbailey
Copy link
Contributor

sbailey commented Feb 3, 2021

@weaverba137 thanks. Let's set the desispec absolute threshold to 25%, and the relative drop threshold to 1% (and that can be done independently of this PR).

Note that although 29% looks quite low, many of the "untested" lines of code are:

  • run as part of nightly testing at NERSC because they require large input data or non-trivial dependencies; or
  • tested via subprocess-launched scripts that coverage doesn't see but would still fail tests if wrong; or
  • qa plotting code (ideally would be tested, but lower priority and messier to meaningfully test without large input file, and there is a lot of unused/deprecated code in there too).

Some day in the glorious future we'll have an in-person documentation and testing hackfest...

@weaverba137
Copy link
Member

OK, I'll implement those settings momentarily. Things like plotting code and other IO-related routines can usually be successfully tested with unittest.mock, but a in-person hackfest is really the preferable venue for that.

@sbailey
Copy link
Contributor

sbailey commented Feb 11, 2021

desihub/specex#36 merged, now merging this required desispec PR and retesting with master.

@sbailey sbailey merged commit 400ff3f into master Feb 11, 2021
@sbailey sbailey deleted the spx_io_refactor branch February 11, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants