Skip to content

Recursive array tools support#238

Closed
SouthEndMusic wants to merge 4 commits intoadrhill:mainfrom
SouthEndMusic:recursive_array_tools_support
Closed

Recursive array tools support#238
SouthEndMusic wants to merge 4 commits intoadrhill:mainfrom
SouthEndMusic:recursive_array_tools_support

Conversation

@SouthEndMusic
Copy link
Contributor

Fixes #237.

@SouthEndMusic
Copy link
Contributor Author

SouthEndMusic commented Apr 3, 2025

@adrhill I implemented trace_input methods in an extension and these are indeed triggered in my test, but the error remains

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (dfb4c7c) to head (740c032).

Files with missing lines Patch % Lines
.../SparseConnectivityTracerRecursiveArrayToolsExt.jl 0.00% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (dfb4c7c) and HEAD (740c032). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (dfb4c7c) HEAD (740c032)
18 6
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #238       +/-   ##
===========================================
- Coverage   93.82%   60.05%   -33.78%     
===========================================
  Files          30       31        +1     
  Lines        1183     1154       -29     
===========================================
- Hits         1110      693      -417     
- Misses         73      461      +388     

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

@SouthEndMusic
Copy link
Contributor Author

SouthEndMusic commented Apr 3, 2025

Hmm it looks like the problem is that e.g.

using RecursiveArrayTools
u = NamedArrayPartition(; foo=rand(5))
similar(u, Int)

yields the output

5-element Vector{Int64}:
                   0
 9223372036854775807
                  13
                   0
                  -1

SciML/RecursiveArrayTools.jl#430

@@ -0,0 +1,12 @@
using RecursiveArrayTools
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like this is missing from the test environment.

u = NamedArrayPartition(; foo=rand(5))
du = copy(u)

jacobian_sparsity(f!, du, u, TracerSparsityDetector()) No newline at end of file
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to have a slightly more rigorous test than just a dry-run.

I don't know the functionality of NamedArrayPartition, but the tests should cover as much of it as possible.

@adrhill
Copy link
Owner

adrhill commented Apr 3, 2025

So ArrayPartition are always flat vectors? Does this support scalar values, e.g. ArrayPartition(rand(2,2), 1.0, rand(3))?

@SouthEndMusic
Copy link
Contributor Author

Closing this PR as this was fixed by SciML/RecursiveArrayTools.jl#431.

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.

Add support for RecursiveArrayTools.jl

3 participants