_copy_output() to replace Base.copy() usage.

[AstitvaAggarwal]

Second PR - Avoid type piracy and define a custom function to copy outputs of supported types. Refer #517 .

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
src/interface.jl	97.45% <100.00%> (+0.75%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AstitvaAggarwal]

@willtebbutt I'll start small with this function as Base.copy() is used in multiple places. If it works for all test cases in prepare_pullback_cache() and value_and_pullback() . I shall switch it for all the rrules, src files. Obviously the name _copy_temp() is too generic, i expect to change it once i get the functionality right.

[AstitvaAggarwal]

@willtebbutt I think the test failures are unrelated, they even spring up on PR #531 . All my tests pass locally. Also turns out Base.deepcopy was failing at an edge case so ive opened up an Issue and PR JuliaLang/julia#57882 for that as well, Base.deepcopy and my function should both work.

[willtebbutt]

Once #531 is merged, you should just need to merge the changes from that PR into this one, and CI should start passing again.

[willtebbutt]

If you sync this branch with main, it should clear up any errors caused by the issue with AllocCheck.

[AstitvaAggarwal]

Although all tests pass here (as expected) I'm still working on the edge case PR for Base.deepcopy() so once that is finalized i will add that logic in as well, for now this code should pass all the given test cases. Also the number of commits in between kinda helped me figure out the bug in deepcopy 😂.

[yebai]

Do we still miss anything here?

[AstitvaAggarwal]

@yebai No not really, just waiting for @willtebbutt to take a look.

[willtebbutt]

Just a few minor things.

shall switch it for all the rrules, src files. Obviously the name _copy_temp() is too generic, i expect to change it once i get the functionality right.

I actually think we probably don't want to switch it out elsewhere. Base.copy is fine most of the time, it's just for this very specific use-case that something more specific is needed I believe.

[willtebbutt]

I think the only thing left to do here is to sort the name of the function out. As you say, something other than _copy_temp is probably needed.

What do you think about _copy_output, since we only ever use it to copy the output of a function? It's not an amazing name, but we can always change it later since we won't be making it part of the public interface.

[AstitvaAggarwal]

yeah this sounds okay.

[willtebbutt]

LGTM. Thanks for this!