Remove openmp from MatrixMultiplication function

[muhammadalhroob]

This Pull request:

Cleaning up TMatrixT.cxx; As discussed in PR #20365, I removed OpenMP pragmas.

Changes or fixes:

Checklist:

• tested changes locally
• updated the docs (if necessary)

This PR fixes #

[guitargeek]

Hi! @muhammadalhroob, thanks for the PR!

Same request as before: can you make sure there are no merge commits in your development branch?

Only allowing fast-forward merges (i.e. merging only branches that didn't do any merged themselves) in GitHub helps us to maintain a clean and linear project history without unnecessary merge commits. This approach simplifies the commit history, making it easier to understand and manage.

[silverweed]

Is it really sufficient to remove those lines? Meaning: does the rest of the code make sense at all without openMP to accelerate it? Are the changes still a performance improvement with this removal?

Also, why are we not using nb anymore at all? Are we now assuming that the second matrix has the correct amount of elements without checking?

[guitargeek]

Is it really sufficient to remove those lines? Meaning: does the rest of the code make sense at all without openMP to accelerate it? Are the changes still a performance improvement with this removal?

According to #20365 (comment), the speedup did not rely on OpenMP.

Also, why are we not using nb anymore at all? Are we now assuming that the second matrix has the correct amount of elements without checking?

Yes. The previous implementation also didn't make any checks by the way. It it actually quite common to have nb implicit, also in the BLAS libraries (compare also the CBLAS backend for TMatrix). I guess that's why the new code also doesn't use it, now that we know it's AI generated it explains some things 🙂

[muhammadalhroob]

Hi @guitargeek,
I started from the updated master branch, I git cloned root after I synchronised my fork, then I made the remove-openmp-matrixmult branch. I am not sure how to proceed and avoid the 'merge commits in your development branch'

[silverweed]

Yes. The previous implementation also didn't make any checks by the way. It it actually quite common to have nb implicit, also in the BLAS libraries (compare also the CBLAS backend for TMatrix). I guess that's why the new code also doesn't use it, now that we know it's AI generated it explains some things 🙂

Since the caller needs to do the work of passing in the number anyway it would be nice to have an assert on that imho

[guitargeek]

How did you "synchronised my fork"? It looks like you tried to do that by merging the master branch of root into the master branch of your fork:
https://github.com/muhammadalhroob/root/commits/master/

That's not synced at all, because the histories are very different, as the commits are in different order with the merges. When syncing the master branch in your fork, make sure your forks master a clean "copy" of the master in ROOT.

I usually make sure of that by checking out the latest root-project/master branch in my local clone of the repository, and then force-push to the master of my fork.

An alternative is to ignore the master branch in your fork completely. You just check out the root-project/master branch and create your development branch from there.

Also a general advice: nothing good ever comes out of git merge, if you don't have very particular uscases and you know what I'm doing. I personally have not used it in 15 years. The better way to sync branches is git rebase.

There is plenty of discussion on git merge vs git rebase on the internet too, some people are very passionate about it. I think it's an important concept to understand.

[silverweed]

Also, whenever you name a commit think of the people who are gonna read it in the context of the repo's history. "Rebase after pulling from root master" gives zero useful information. If you have a single-commit PR it's customary to have your commit short message to be the same as the PR title (ideally with the long message containing the explanation of what's happening and why)

[guitargeek]

Thanks! I'll reword the commit on merging.