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

Go back from BangBang convention to enforcing mutation? #168

Closed
gdalle opened this issue Apr 10, 2024 · 3 comments · Fixed by #169
Closed

Go back from BangBang convention to enforcing mutation? #168

gdalle opened this issue Apr 10, 2024 · 3 comments · Fixed by #169
Labels
core Related to the core utilities of the package

Comments

@gdalle
Copy link
Owner

gdalle commented Apr 10, 2024

Just to keep track of the discussion with @devmotion: SciML/OptimizationBase.jl#31 (comment)

@gdalle gdalle added the core Related to the core utilities of the package label Apr 10, 2024
@gdalle
Copy link
Owner Author

gdalle commented Apr 11, 2024

Main problems spotted:

@devmotion
Copy link

I assume DiffResults predates the bang bang convention. But similar to the discussion in the other thread IMO this behaviour (note that these examples are for immutable arrays!) is not relevant for the point I'm trying to make: When you require/assume mutable containers, then you should directly use the more direct and more efficient inplace function. Bang bang is useful for generic code that accepts both mutable and immutable arrays.

@gdalle
Copy link
Owner Author

gdalle commented Apr 11, 2024

The code in DI is trying to be as generic as possible, but I realized yesterday that every downstream user, myself included, would have to make heavy use of copyto!(x, operator!!(x)) which is suboptimal, so I am indeed working to change that

@gdalle gdalle linked a pull request Apr 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to the core utilities of the package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants