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

Eliminate legacy interface and obsolete large_data option #755

Merged
merged 18 commits into from
Sep 22, 2023

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Sep 15, 2023

Those interfaces were already largely discouraged. The large_data infrastructure was incomplete and buggy, while the legacy interface was very slow.
This PR is in preparation of our next major release, where we can afford breaking some minor things. It eliminates these two mechanisms, making the code lighter and avoiding confusion in users.
While doing this, I had to also revise a little the mechanism of save_all, that needed the legacy interface and allowed to load all the sub-periodograms of the averaged periodogram in memory. It was used also from _create_matrix in DynamicalPowerspectrum. The result is not bad, because to do this we are basically avoiding creating a bunch of AveragedPowerspectrum object for each sub spectrum.

@matteobachetti matteobachetti marked this pull request as draft September 15, 2023 13:17
@matteobachetti matteobachetti force-pushed the eliminate_legacy_interface branch 2 times, most recently from e043801 to 83a4b9b Compare September 15, 2023 20:51
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #755 (524d846) into main (c89490b) will decrease coverage by 4.13%.
The diff coverage is 100.00%.

❗ Current head 524d846 differs from pull request most recent head 2355c56. Consider uploading reports for the commit 2355c56 to get more accurate results

@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
- Coverage   97.14%   93.01%   -4.13%     
==========================================
  Files          42       41       -1     
  Lines        7941     7387     -554     
==========================================
- Hits         7714     6871     -843     
- Misses        227      516     +289     
Files Changed Coverage Δ
stingray/__init__.py 100.00% <ø> (ø)
stingray/utils.py 94.47% <ø> (-4.03%) ⬇️
stingray/crossspectrum.py 99.00% <100.00%> (+0.78%) ⬆️
stingray/fourier.py 99.76% <100.00%> (+0.01%) ⬆️
stingray/powerspectrum.py 99.67% <100.00%> (+0.22%) ⬆️

... and 12 files with indirect coverage changes

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

@matteobachetti matteobachetti force-pushed the eliminate_legacy_interface branch from 6908e95 to 5f876b3 Compare September 20, 2023 17:15
Copy link
Member

@dhuppenkothen dhuppenkothen left a comment

Choose a reason for hiding this comment

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

Looking good! Just some questions. :)

stingray/crossspectrum.py Outdated Show resolved Hide resolved
stingray/crossspectrum.py Show resolved Hide resolved
stingray/crossspectrum.py Show resolved Hide resolved
stingray/fourier.py Show resolved Hide resolved
stingray/powerspectrum.py Show resolved Hide resolved
stingray/tests/test_crossspectrum.py Show resolved Hide resolved
stingray/tests/test_crossspectrum.py Show resolved Hide resolved
@matteobachetti matteobachetti force-pushed the eliminate_legacy_interface branch from 2fc764e to 05b70a8 Compare September 22, 2023 08:45
Copy link
Member

@dhuppenkothen dhuppenkothen left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

One comment: I wonder whether for the changelog, there should be a more substantial message that this is a breaking change in the next major stingray version?

@matteobachetti matteobachetti force-pushed the eliminate_legacy_interface branch from 05b70a8 to 524d846 Compare September 22, 2023 09:05
@matteobachetti
Copy link
Member Author

Good point. I now marked the change as breaking in the towncrier snippet!

@matteobachetti matteobachetti added this pull request to the merge queue Sep 22, 2023
Merged via the queue into main with commit e3acafe Sep 22, 2023
@matteobachetti matteobachetti deleted the eliminate_legacy_interface branch September 22, 2023 09:35
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.

2 participants