Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_copy_temp() to replace Base.copy() usage. #529

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

AstitvaAggarwal
Copy link
Collaborator

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

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/interface.jl 97.50% <100.00%> (+0.79%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AstitvaAggarwal
Copy link
Collaborator Author

AstitvaAggarwal commented Mar 21, 2025

@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
Copy link
Collaborator Author

AstitvaAggarwal commented Mar 25, 2025

@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
Copy link
Member

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
Copy link
Member

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

@AstitvaAggarwal
Copy link
Collaborator Author

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

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.

None yet

2 participants