Skip to content

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Apr 29, 2025

This diverges from e.g. scipy’s default behavior, because _csbase.toarray has "C" as default, whereas numpy has "K" as default, and I think the behavior should be the same in both cases.

Therefore CSC matrices become F-contiguous arrays when order="K" (the default).

Dask is problematic, because it doesn’t allow to specify order at all: It basically does what it wants, so we can’t really test for sane behavior.

Copy link

codspeed-hq bot commented Apr 29, 2025

CodSpeed Performance Report

Merging #94 will not alter performance

Comparing pa/order (3cc4b22) with main (2a0d323)

Summary

✅ 160 untouched benchmarks

@flying-sheep flying-sheep marked this pull request as ready for review May 5, 2025 13:41
@flying-sheep flying-sheep requested review from ilan-gold and Intron7 May 5, 2025 13:41
Copy link

codecov bot commented May 5, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.45%. Comparing base (60d5afb) to head (3cc4b22).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/fast_array_utils/conv/_to_dense.py 63.15% 7 Missing ⚠️
src/fast_array_utils/types.py 75.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (60d5afb) and HEAD (3cc4b22). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (60d5afb) HEAD (3cc4b22)
4 3
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #94       +/-   ##
===========================================
- Coverage   98.27%   83.45%   -14.83%     
===========================================
  Files          17       17               
  Lines         406      417       +11     
===========================================
- Hits          399      348       -51     
- Misses          7       69       +62     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@flying-sheep flying-sheep added the run-gpu-ci Apply this label to run GPU CI once label May 5, 2025
@github-actions github-actions bot removed the run-gpu-ci Apply this label to run GPU CI once label May 5, 2025
Copy link
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Aside from a bit more info in the public API, looks good

Comment on lines +58 to +59
order
The order of the output array: ``C`` (row-major) or ``F`` (column-major). ``K`` and ``A`` derive the order from ``x``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more info here i.e., what you say in the PR comment

Copy link
Member Author

@flying-sheep flying-sheep May 6, 2025

Choose a reason for hiding this comment

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

@flying-sheep flying-sheep merged commit 1db1f50 into main May 6, 2025
10 of 13 checks passed
@flying-sheep flying-sheep deleted the pa/order branch May 6, 2025 08:30
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