Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Apr 21, 2025

Change elty1 to elty2 to avoid repeating the condition.

@codecov
Copy link

codecov bot commented Apr 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.12%. Comparing base (dcf579c) to head (e883422).
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1294   +/-   ##
=======================================
  Coverage   92.12%   92.12%           
=======================================
  Files          34       34           
  Lines       15509    15509           
=======================================
  Hits        14287    14287           
  Misses       1222     1222           

☔ 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.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Does that reduce the number of cases and hence test runtime?

@jishnub
Copy link
Member Author

jishnub commented Apr 21, 2025

Actually, this increases the number of cases. Currently, e.g. for elty1 = Float64, the only combination being tested is (elty1, elty2) = (Float64, Float64), as the condition elty1 ∈ (ComplexF32, Int) is not satisfied. This PR would test other cases as well, with the cases tested being

(elty1, elty2) = (Float64, Float64)
(elty1, elty2) = (Float64, ComplexF32)
(elty1, elty2) = (Float64, Int64)

This increases the runtime by quite a bit.

Reading the comment above, the intent appears to be to test one combination of differing eltypes, whereas we are testing two here. Perhaps we may only loop over one eltype instead of two by changing the condition here.

jishnub added 3 commits April 21, 2025 22:43
Change `elty1` to `elty2` within the `elty2` loop so that the condition isn't repeated.
@jishnub jishnub changed the title Fix variable name in testtriag.jl loop Test two combinations in triangular test sets Apr 21, 2025
@jishnub
Copy link
Member Author

jishnub commented Apr 21, 2025

The current approach ensures that exactly two combinations are tested for each type. This changes the current behaviour of testing all combinations for Int and ComplexF32. This reduction in the number of combinations being tested reduces the runtime by several minutes (around 10 minutes, looking at the test run times).

@jishnub
Copy link
Member Author

jishnub commented Apr 22, 2025

@andreasnoack since you were involved in splitting the test sets, do you think this approach seems reasonable? In this PR, we test each type against itself, and another type that is chosen at random.

@jishnub jishnub merged commit b5cc56a into master Apr 22, 2025
4 checks passed
@jishnub jishnub deleted the jishnub-patch-1 branch April 22, 2025 14:34
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.

3 participants