Skip to content

Simplify scale #3351

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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Simplify scale #3351

wants to merge 30 commits into from

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Nov 11, 2024

scale_sparse has a lot of code that’s not needed.

This PR also fixes zappy compatibility for scale, not that anyone is using it as far as I know.

Copy link

codecov bot commented Nov 11, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1675 1 1674 426
View the top 1 failed test(s) by shortest run time
tests/test_preprocessing.py::test_scale
Stack Traces | 0.036s run time
def test_scale():
        adata = pbmc68k_reduced()
        adata.X = adata.raw.X
        v = adata[:, 0 : adata.shape[1] // 2]
        # Should turn view to copy https://github..../anndata/issues/171#issuecomment-508689965
        assert v.is_view
        with pytest.warns(Warning, match="view"):
            sc.pp.scale(v)
        assert not v.is_view
>       assert_allclose(v.X.var(axis=0), np.ones(v.shape[1]), atol=0.01)

#x1B[1m#x1B[31mtests/test_preprocessing.py#x1B[0m:342: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = (<function assert_allclose.<locals>.compare at 0x7f5855bce3e0>, matrix([[0.9985714318, 0.9985714246, 0.9985714244, 0.9...1.,
       1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,
       1., 1., 1., 1., 1., 1., 1., 1.]))
kwds = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=1e-07, atol=0.01', 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
#x1B[1m#x1B[31mE           AssertionError: #x1B[0m
#x1B[1m#x1B[31mE           Not equal to tolerance rtol=1e-07, atol=0.01#x1B[0m
#x1B[1m#x1B[31mE           #x1B[0m
#x1B[1m#x1B[31mE           (shapes (1, 382), (382,) mismatch)#x1B[0m
#x1B[1m#x1B[31mE            x: matrix([[0.998571, 0.998571, 0.998571, 0.998571, 0.998571, 0.998571,#x1B[0m
#x1B[1m#x1B[31mE                    0.998571, 0.998571, 0.998571, 0.998571, 0.998571, 0.998571,#x1B[0m
#x1B[1m#x1B[31mE                    0.998571, 0.998571, 0.998571, 0.998571, 0.998571, 0.998571,...#x1B[0m
#x1B[1m#x1B[31mE            y: array([1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,#x1B[0m
#x1B[1m#x1B[31mE                  1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,#x1B[0m
#x1B[1m#x1B[31mE                  1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,...#x1B[0m

#x1B[1m#x1B[.../hostedtoolcache/Python/3.11.11.../x64/lib/python3.11/contextlib.py#x1B[0m:81: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link

scverse-benchmark bot commented Nov 11, 2024

Benchmark changes

Change Before [329ad66] After [233d8a3] Ratio Benchmark (Parameter)
+ 5.48±0.06ms 6.42±0.1ms 1.17 preprocessing_counts.FastSuite.time_calculate_qc_metrics('pbmc68k_reduced', 'counts')
- 6.59±0.4ms 5.08±0.02ms 0.77 preprocessing_counts.FastSuite.time_normalize_total('bmmc', 'counts-off-axis')
+ 955M 1.31G 1.37 preprocessing_log.peakmem_scale('pbmc3k', 'off-axis')
+ 1.15G 1.5G 1.3 preprocessing_log.peakmem_scale('pbmc3k', None)
+ 21.6±0.07ms 27.2±0.2ms 1.26 preprocessing_log.time_highly_variable_genes('pbmc3k', 'off-axis')
- 763±30ms 561±70ms 0.74 preprocessing_log.time_scale('pbmc3k', 'off-axis')

Comparison: https://github.com/scverse/scanpy/compare/329ad666f280b3e7335b3735a04a0a97f430e6f2..233d8a346423bea0043baef72595adf3fddc3d3e
Last changed: Mon, 14 Apr 2025 14:23:48 +0000

More details: https://github.com/scverse/scanpy/pull/3351/checks?check_run_id=40505086063

@flying-sheep flying-sheep changed the title Simplify-scale Simplify scale Nov 11, 2024
@flying-sheep flying-sheep modified the milestones: 1.11.0, 1.12.0 Dec 20, 2024
@flying-sheep flying-sheep self-assigned this Feb 17, 2025
@flying-sheep flying-sheep modified the milestones: 1.12.0, 1.11.1 Feb 17, 2025
@flying-sheep flying-sheep marked this pull request as ready for review February 17, 2025 11:57
@flying-sheep flying-sheep requested review from Intron7 and removed request for Intron7 February 17, 2025 16:31
@flying-sheep flying-sheep requested a review from Intron7 February 20, 2025 15:09
@flying-sheep
Copy link
Member Author

@Intron7 any idea what I did wrong? Why do the benchmarks say that it uses more memory?

@flying-sheep
Copy link
Member Author

@Intron7 ping

@flying-sheep flying-sheep modified the milestones: 1.11.1, 1.11.2 Mar 31, 2025
@flying-sheep flying-sheep requested a review from ilan-gold April 3, 2025 13:39
Copy link
Member

@Intron7 Intron7 left a comment

Choose a reason for hiding this comment

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

This would make the default code path for csr even faster. otherwise LGTM

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.

3 participants