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

MNT: Infrastructure and other updates #202

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

pllim
Copy link
Member

@pllim pllim commented Dec 5, 2023

I stared into the code and I couldn't unsee things, so here it is.

Unable to handle this one until I get access:

After merge, can add new job names to branch protection.

@pllim pllim added the Extra CI Run cron jobs on PR label Dec 5, 2023
@pllim pllim force-pushed the infra-clearing-house branch 2 times, most recently from 3e10373 to 6b14578 Compare December 5, 2023 22:02
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (aa4ceab) 78.83% compared to head (bbf377d) 81.32%.

Files Patch % Lines
specreduce/fluxcal.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   78.83%   81.32%   +2.49%     
==========================================
  Files          10       10              
  Lines         992      996       +4     
==========================================
+ Hits          782      810      +28     
+ Misses        210      186      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Replace broken URL and deprecated np.trapz usage.

Ignore link that is blocking CI check.
@pllim pllim marked this pull request as ready for review December 5, 2023 22:49
@pllim

This comment was marked as resolved.

@pllim pllim closed this Dec 5, 2023
@pllim pllim reopened this Dec 5, 2023
@@ -1,21 +1,16 @@
[tox]
envlist =
py{310,311,312}-test{,-devdeps,-predeps}{,-cov}
build_docs
py{38,39,310,311,312}-test{,-alldeps}{,-oldestdeps,-devdeps,-predeps}{,-cov}
Copy link
Contributor

Choose a reason for hiding this comment

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

we have discussed and agreed that we should require >=3.10 so that we can use the improved type annotation capabilities. this change is still staged in #182, but it would make more sense to wrap that into this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even numpy has not dropped Python 3.9 yet. Are you sure you want to do it so soon?

https://github.com/numpy/numpy/blob/c9340c210666afefb6946fa6f3a105856ff0e418/pyproject.toml#L19

Copy link
Contributor

Choose a reason for hiding this comment

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

well, numpy has a much, much larger installed base to support and we don't. 3.10 isn't all that new so it's not a huge ask. if one cares at all about performance, they really want 3.11+, anyway.

Copy link
Contributor

@tepickering tepickering left a comment

Choose a reason for hiding this comment

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

this is so awesome! so many cleanups and fixes i've wanted to see, but never got around to. the migration to pyproject.toml is also extremely welcome. my only niggle is that we made the decision on slack 7 months ago to set python_requires to >=3.10. this was implemented in #182, but not yet merged into main. i'm fine with merging this as-is and rebasing/revising #182 accordingly. otoh, this is probably the more sensible PR to make that change given the rest of the changes its making to package config/infrastructure.

@pllim
Copy link
Member Author

pllim commented Dec 7, 2023

Sorry about the conflict with #182 . I don't watch this repo so I was not aware of it. I replied in #202 (comment) (I think it is a little too soon but I am also not a specreduce maintainer).

Whether you want to merge this first, or cherry-pick your changes to this PR and take this over, I will leave to you.

@tepickering
Copy link
Contributor

tepickering commented Dec 7, 2023

no worries on the conflicts. i would say let's merge this ASAP and i'll do what i need to do to clean up #182. it's the code changes that i've made in #182 that require 3.10+, iirc. if we want to stick with bumping the requirement there vs here, i'm fine with that. interested in @kecnry's take.

@kecnry
Copy link
Member

kecnry commented Dec 7, 2023

I don't have any strong opinions on the order of operations - good to merge on my end! Thanks for all the improvements @pllim !

@pllim
Copy link
Member Author

pllim commented Dec 8, 2023

Then, feel free to merge! I am tempted to merge myself but I shouldn't. Thanks!

@kecnry kecnry merged commit 3ec660a into astropy:main Dec 11, 2023
14 checks passed
@pllim pllim deleted the infra-clearing-house branch December 11, 2023 16:17
tepickering added a commit to tepickering/specreduce that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra CI Run cron jobs on PR
Projects
None yet
3 participants