Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.46%. Comparing base (8e805ef) to head (0f20455).
Report is 58 commits behind head on master.

@@ Coverage Diff @@
## master #451 +/- ##
==========================================
+ Coverage 93.18% 93.46% +0.28% 
==========================================
 Files 52 51 -1 
 Lines 1261 1255 -6 
==========================================
- Hits 1175 1173 -2 
+ Misses 86 82 -4 

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

Huh, this is weird. Is it clear if we're hitting these adjoints at all in our tests?

Huh, this is weird. Is it clear if we're hitting these adjoints at all in our tests?

_map is used by the transforms, and most of the transforms have passing AD tests, so I would say "probably"? I've not yet added print() statements to the custom adjoints to actually see...

_map is used by the transforms, and most of the transforms have passing AD tests, so I would say "probably"?

The rules were defined for map, not _map.

_map is used by the transforms, and most of the transforms have passing AD tests, so I would say "probably"?

The rules were defined for map, not _map.

The rules were defined for map(t::Transform, x), which is defined in src/transform/transform.jl as _map(t, x). So what are you trying to say by that?

That

Just verified this locally by adding print statements to the zygote rules -- they don't seem to be hit. Given that we document that one can call map(t, x), I would be in favour of adding a couple of unit tests to ensure that this actually works. We don't need to do that here, but an issue to this effect so that we don't forget about it would be good.

@devmotion ah okay, I think I see what you mean now.

Given the map(t::Transform, x) = _map(t, x) definition, should we then just replace all _map calls inside e.g. kernelmatrix methods with just map?

@willtebbutt what would you like to have added unit tests for ?

Given the map(t::Transform, x) = _map(t, x) definition, should we then just replace all _map calls inside e.g. kernelmatrix methods with just map?

I don't think this will work. The main reason why _map was introduced was that map did not work because Zygote handles map(f, ::AbstractVector). See #113, #114, and FluxML/Zygote.jl#646.