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

Allow for index arrays in ShermanMorrison #356

Merged
merged 11 commits into from
Nov 17, 2023

Conversation

vhaasteren
Copy link
Member

@vhaasteren vhaasteren commented Sep 22, 2023

This one solves #319 by allowing EcorrKernelNoise to work with index arrays, rather than slice objects. The function that is changed now is quant2ind, which now by default returns an list of index arrays.

The fastshermanmorrison package with Cython code is also compatible with this change, in case anyone is using that. Just upgrade it with pip

Thanks to Bjorn Larsen for providing a first outline of this modification.

TODO:

  • Modify quant2ind to return index arrays (and slices)
  • Fix failing tests in test_white_signals
  • Change _setup_sparse and _get_ndiag_sparse in EcorrKernelNoise to allow for index arrays
  • Update fastshermanmorrison-pulsar for full compliance
  • Added unit tests that shuffle TOAs for EcorrKernelNoise

Depends on #352 for codev report

@vhaasteren vhaasteren changed the title Allow for index arrays in ShermanMorrison [WIP] Allow for index arrays in ShermanMorrison Sep 22, 2023
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Merging #356 (b9a3bcc) into dev (2415762) will increase coverage by 0.07%.
The diff coverage is 96.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #356      +/-   ##
==========================================
+ Coverage   88.35%   88.42%   +0.07%     
==========================================
  Files          13       13              
  Lines        3023     3033      +10     
==========================================
+ Hits         2671     2682      +11     
+ Misses        352      351       -1     
Files Coverage Δ
enterprise/signals/utils.py 86.77% <100.00%> (+0.31%) ⬆️
enterprise/signals/white_signals.py 98.62% <100.00%> (+0.02%) ⬆️
enterprise/signals/signal_base.py 90.14% <94.44%> (+0.03%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@vhaasteren vhaasteren changed the title [WIP] Allow for index arrays in ShermanMorrison Allow for index arrays in ShermanMorrison Sep 25, 2023
@vhaasteren vhaasteren marked this pull request as draft September 25, 2023 06:49
@vhaasteren vhaasteren marked this pull request as ready for review September 28, 2023 12:56
@vhaasteren vhaasteren changed the base branch from master to dev November 7, 2023 08:55
@vhaasteren vhaasteren self-assigned this Nov 7, 2023
@vhaasteren vhaasteren linked an issue Nov 7, 2023 that may be closed by this pull request
@vhaasteren
Copy link
Member Author

@AaronDJohnson, unit tests for the extra lines are done. As commented on the code, the remaining two lines that aren't covered have never been covered and IMHO shouldn't even exist. I vote to leave those like this

@AaronDJohnson AaronDJohnson merged commit d782c29 into nanograv:dev Nov 17, 2023
13 checks passed
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.

Issue with EcorrKernelNoise for ultra-wideband data
2 participants