Skip to content

Consolidate linalg and VI tests#469

Open
kshyatt wants to merge 2 commits into
mainfrom
ksh/enz_consolidate
Open

Consolidate linalg and VI tests#469
kshyatt wants to merge 2 commits into
mainfrom
ksh/enz_consolidate

Conversation

@kshyatt

@kshyatt kshyatt commented Jun 30, 2026

Copy link
Copy Markdown
Member

Fixes to the caching for CI runs and some Enzyme improvements have cut the runtime of Enzyme tests substantially, so we can probably fold these back into smaller files to reduce the amount of CI runs.

@kshyatt kshyatt force-pushed the ksh/enz_consolidate branch from ff1526d to 0f9729f Compare July 1, 2026 14:08
@kshyatt kshyatt force-pushed the ksh/enz_consolidate branch from 0f9729f to 4396568 Compare July 1, 2026 14:13
Comment thread test/enzyme/linalg.jl Outdated
Comment on lines +29 to +35
rTαs = if α === Zero()
(Const,)
elseif !is_ci
(Active, Const)
else
(Active,)
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
rTαs = if α === Zero()
(Const,)
elseif !is_ci
(Active, Const)
else
(Active,)
end
rTαs = []
α === Zero() || push!(rTαs, Active)
=== Zero() || !is_ci) && push!(rTαs, Const)

Just a small suggestion for code style, I tend to find it a bit easier to see what is going on if there aren't too many lines

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't quite like that this is now changed just here in this one instance, while the test file is full of these constructs. I don't mind the three case separation and keeping rTαs a tuple (though clearly that doesn't have any real advantages here). I would suggest to revert it to the previous state, unless @lkdvos feels strongly, but then they should probably be changed everywhere

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kind of meant to change it everywhere but got pulled into a meeting midway through my suggestions 😁

Co-authored-by: Lukas Devos <ldevos98@gmail.com>
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
see 6 files with indirect coverage changes

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

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