-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add in-place methods to SimpleBase #149
base: SNAPSHOT
Are you sure you want to change the base?
Conversation
Hey @BluSteve, Personally, I don't see the advantage of putting all these in-place methods on the Simple interface. |
I just saw you already discussed my concern in the previous PR. |
Honestly it does have a pretty niche use case, and I would understand if the PR was rejected. The main use would be as a direct find-and-replace for the pattern x=x+a. In my experience, scalar operations benefit most because they aren't slow enough for the memory allocation bottleneck to disappear and they're also intuitive since it's just x+=a. I personally don't think it's that much of a mental overhead, but then again I am used to JBlas syntax. |
Mentioned this in the other thread, but basically I think now is a good time reconsidering how simple SimpleMatrix is. Most similar libraries have a lot more functions and it is easier to find operations when everything is derived from a single class. Although SimpleMatrix was very much a reaction against that paradigm. Maybe the way to handle this is to have @BluSteve experiment with new designs in the "experimental" package that's not included by default. Then we can consider moving some new operations from there into SimpleMatrix at a later date. That way we don't need to guess what's best right now and can have a less strict PR process. |
Any random people looking at this PR have any thoughts on adding a lots of new functions for in place operations? Main argument against is all the complexity it would add and most functions in SimpleMatrix return a new instance so people would need to be careful. |
One thing worth noting is that the CommonOps transpose method apparently does not work in-place.