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

Always use astropy's sigma clipping and default to string-based functions #794

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

astrofrog
Copy link
Member

This switches the sigma clipping to always use the astropy sigma clipping, and default to using strings for the statistics/functions to use by default as this provides the best performance. This requires bumping the minimum version of astropy to 4.3 which I hope should be ok now that LTS is now 5.x?

I haven't done extensive testing beyond the built-in test suite and my own use case, but for my case the sigma clipping became almost 3x faster.

  • For new contributors: Did you add yourself to the "Authors.rst" file?
  • Did you add an entry to the "Changes.rst" file?

Fixes #793

I also think #753 could be closed with this merged.

Perhaps #624 would also be resolved by this? (but make sure you change the functions for sigma clipping to be strings)

@astrofrog
Copy link
Member Author

astrofrog commented Aug 18, 2022

I wasn't sure what major version this should go in in the changelog or what to milestone it as?

@mwcraig
Copy link
Member

mwcraig commented Aug 26, 2022

Sorry for the delay @astrofrog, and thanks for opening -- this is either 2.4 if use_astropy gives a deprecation warning or 3.0 if it deosn't. I'm inclined towards 2.4, and will push a check for use_astropy by Monday, then merge and release.

Might even get to it today....

@mwcraig mwcraig added this to the 2.4 milestone Aug 26, 2022
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #794 (88f1d6e) into main (4ae834d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   97.43%   97.48%   +0.04%     
==========================================
  Files           9        9              
  Lines        1402     1390      -12     
==========================================
- Hits         1366     1355      -11     
+ Misses         36       35       -1     
Impacted Files Coverage Δ
ccdproc/combiner.py 94.62% <100.00%> (+0.12%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mwcraig
Copy link
Member

mwcraig commented Aug 26, 2022

Thanks @astrofrog, merging!

@mwcraig mwcraig merged commit 0119f41 into astropy:main Aug 26, 2022
@astrofrog
Copy link
Member Author

@mwcraig - just to check, are you planning a release soon to include this change? (just to know whether to work around this for now or whether to just wait).

@mwcraig
Copy link
Member

mwcraig commented Nov 8, 2022

Would a release by early next week work for you? I should be able to get to it by then....

@astrofrog
Copy link
Member Author

Yes sounds good! Thank you!

@mwcraig
Copy link
Member

mwcraig commented Nov 17, 2022

@astrofrog -- I released 2.4.0, which includes this PR, last night. It is on PyPI and should be on conda-forge shortly, announcement coming later today or tomorrow.

@astrofrog
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for passing strings for cenfunc and stdfunc for sigma clipping
2 participants