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

Add in-place methods to SimpleBase #149

Open
wants to merge 3 commits into
base: SNAPSHOT
Choose a base branch
from

Conversation

BluSteve
Copy link
Contributor

One thing worth noting is that the CommonOps transpose method apparently does not work in-place.

@FlorentinD
Copy link
Collaborator

Hey @BluSteve,
Is there a reason why you need to use the Simple interface?
If you want more control, you could directly use the CommonOps_DDRM methods instead of the simple interface.

Personally, I don't see the advantage of putting all these in-place methods on the Simple interface.
I would even argue, this makes it harder for beginners as its yet another concept they need to think of.

@FlorentinD
Copy link
Collaborator

FlorentinD commented Nov 26, 2021

I just saw you already discussed my concern in the previous PR.

@BluSteve
Copy link
Contributor Author

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.

@lessthanoptimal
Copy link
Owner

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.

@lessthanoptimal
Copy link
Owner

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.

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

3 participants