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

[pre-commit.ci] pre-commit autoupdate #830

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pre-commit-ci[bot]
Copy link
Contributor

@pre-commit-ci pre-commit-ci bot commented Dec 23, 2024

Copy link

codspeed-hq bot commented Dec 23, 2024

CodSpeed Performance Report

Merging #830 will degrade performances by 23.2%

Comparing pre-commit-ci-update-config (53b6fe0) with main (ada7871)

Summary

❌ 1 (👁 1) regressions
✅ 339 untouched benchmarks

Benchmarks breakdown

Benchmark main pre-commit-ci-update-config Change
👁 test_index_slice[side=100-rank=2-format='gcxs'] 2.4 ms 3.1 ms -23.2%

hameerabbasi
hameerabbasi previously approved these changes Dec 23, 2024
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.8.3 → v0.8.6](astral-sh/ruff-pre-commit@v0.8.3...v0.8.6)
@hameerabbasi
Copy link
Collaborator

@willow-ahrens @kylebd99 There are some new failing array API tests for Finch -- could you take a look?

@kylebd99
Copy link

kylebd99 commented Jan 7, 2025

I'm taking a look now, but I'm having trouble parsing the testing infrastructure. Do you know what the input, program, and expected outputs are for the failing tests?

@hameerabbasi
Copy link
Collaborator

@kylebd99 these are the three failures:

FAILED array_api_tests/test_special_cases.py::test_binary[pow(x1_i is -infinity and x2_i < 0 and not (x2_i.is_integer() and x2_i % 2 == 1)) -> +0]
FAILED array_api_tests/test_special_cases.py::test_binary[__pow__(x1_i is -infinity and x2_i < 0 and not (x2_i.is_integer() and x2_i % 2 == 1)) -> +0]
FAILED array_api_tests/test_special_cases.py::test_iop[__ipow__(x1_i is -infinity and x2_i < 0 and not (x2_i.is_integer() and x2_i % 2 == 1)) -> +0]

They're all about scalars x1 ** x2:

  1. inf ** x2 with even x2 < 0 must be +0.
  2. (-inf) ** x2 with even x2 < 0 must be +0.
  3. x1 ** (-inf) with odd x1 < 0 must be +0.

In general the special cases produce +0 instead of -0 if ambiguous.

@willow-ahrens
Copy link
Collaborator

willow-ahrens commented Jan 8, 2025

@hameerabbasi, in the contributing guide, it would be really helpful to have a set of commands that runs all of these tests. Not just pytest --pyargs sparse, but something along the lines of:

git clone [email protected]:pydata/sparse.git
python -m venv venv
source venv/bin/activate
pip install ./sparse [all] #or however we're supposed to do this locally
git clone array_api_tests #array api tests, how do they work?
pip install ./array_api_tests #array api tests, how do they work?
pytest --pyargs sparse
pytest --pyargs array_api #array api tests, how do they work?

I think something like the above commands would be relatively easy for you to write, but this takes several hours for us to figure out every time. Thanks for your help and advice with this!

On the issue itself: +0.0and -0.0 are hard to handle correctly in sparse compilers, I've written about it here finch-tensor/Finch.jl#686 and here finch-tensor/Finch.jl#383. The summary is that we have two inconsistent semantics for -0.0:

  • -0.0 + 0.0 = 0.0 (from floating point semantics)
  • -0.0 + 0.0 = -0.0 (from x + 0 = x sparsity rule)
    In solving this for plus, I am tempted to let the annihilation rule win whenever we concern ourselves with fill values (Tensor(0.0) - Tensor(0.0) = Tensor(-0.0)). The runtime values can follow ieee since they will be stored explicitly anyway. If we don't like this, I can add special rules for - and avoid converting a - b into a + (- b).

Finch doesn't even have a sparsity rule for exponentiation, so whatever answer you're getting now is almost surely whatever julia decided to do for floating point semantics in julia. There is no recommendation from IEEE 754 for exponentiation, but strangely julia and python agree on the behavior. Is this instead testing whether the entry is stored explicitly. How can we see the actual test itself? Depending on what behavior we need, I suggest we handle this the same way we handled cld when we needed it not to throw by defining a new operator with the desired behavior (in that case, cld_nothrow.)

A question I'm having now: why did these deciding to fail now? we haven't changed behavior.

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