nanograv / enterprise

ENTERPRISE (Enhanced Numerical Toolbox Enabling a Robust PulsaR Inference SuitE) is a pulsar timing analysis code, aimed at noise analysis, gravitational-wave searches, and timing model analysis.
https://enterprise.readthedocs.io
MIT License
65 stars 67 forks source link

Updating github actions and requirements #257

Closed jellis18 closed 3 years ago

jellis18 commented 3 years ago

This PR does the following:

  1. Revamps github actions tests to run on both linux and osx for python 3.6 - 3.8
  2. Adds a build and release target to github actions. The build will build the wheels and release pushes them to PYPI. I don't know if there is a NANOGrav pypi account but that should probably be used. These build and release actions get triggered on a github release
  3. Added isort pre-commit. This will ensure that imports are always consistent
  4. Replaced print with loggers where appropriate
  5. Removed all __future__ since python 2.7 support is dropped.

There are still a few things that will need to be taken care of down the line. First off, we are still installing libstempo from github and not from a pypi release. I've been working with @vallis on getting a pypi release ready so that will be along soon.

Next, scikit-sparse still requires numpy and cython to be installed beforehand because they don't have the right build dependencies set. I made a PR to fix this but it may not get any attention (the repo hasn't been touched for a few years). Another option is to maybe fork it and use that for our scikit-sparse install (I already did this but it may be better to try to work through the scikit-sparse devs first).

Lastly, all of these changes are mainly meant for the pip path. Once there is a version on pypi we can then make a conda-forge version.

codecov[bot] commented 3 years ago

Codecov Report

Merging #257 (ce403e7) into master (bec52e0) will increase coverage by 0.30%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   85.66%   85.97%   +0.30%     
==========================================
  Files          12       12              
  Lines        2748     2737      -11     
==========================================
- Hits         2354     2353       -1     
+ Misses        394      384      -10     
Impacted Files Coverage Δ
enterprise/signals/selections.py 87.14% <ø> (-0.19%) :arrow_down:
enterprise/signals/white_signals.py 98.41% <ø> (-0.02%) :arrow_down:
enterprise/pulsar.py 91.46% <100.00%> (+0.77%) :arrow_up:
enterprise/signals/signal_base.py 87.65% <100.00%> (+0.56%) :arrow_up:
enterprise/signals/utils.py 84.77% <100.00%> (+0.43%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bec52e0...ce403e7. Read the comment docs.

vallis commented 3 years ago

This looks great Justin. Thank you so much. Several things:

jellis18 commented 3 years ago

@vallis if you just want coverage percent as a check I added that in this PR directly as part of the tests.

As for suitesparse I was unaware there were different builds of the main library itself. I thought it just had to do with whether or not a mkl numpy version was used to build scikit-sparse.

Unfortunately I seems nearly impossible to get mkl-based numpy though pip. I think the best approach may be to leave as is for the pip install path and just use conda if you care about speed.

Doesn't look like conda-forge does anything special for suitesparse

How have you guys handled this in the past?

jellis18 commented 3 years ago

@nanograv/enterprise I don't know how to turn off these checks above that aren't relevant...

Hazboun6 commented 3 years ago

@nanograv/enterprise I don't know how to turn off these checks above that aren't relevant...

I think we can delete them from .github/.codecov.yaml. I assume we could just delete

patch:
      default:
        target: auto
        threshold: 6%

Then there will be no patch coverage check. You are rerunning tests now, so I think those were the tests that weren't passing, is that correct?

jellis18 commented 3 years ago

@Hazboun6 the tests all pass now but yes we could remove that patch coverage check. What I don't know how to get rid of are these image

They don't run on pull requests and I don't even know where "lint" comes from as its not a target

Actually I think I see now. Those were the old targets but aren't there anymore

Hazboun6 commented 3 years ago

Actually I think I see now. Those were the old targets but aren't there anymore

Yeah, @jellis18 is this because GitHub Actions runs the old tests and the new tests in a PR?

Hazboun6 commented 3 years ago

@jellis18 This is super weird. The Action workflow page doesn't even show those checks. Really annoying that we can't circumvent them somehow.

jellis18 commented 3 years ago

@Hazboun6, fixed it