Parallel calculation of PDOS and (fat)bands

[pfebrer]

Now if pathos is available, PDOS and (fat)bands calculated from a hamiltonian are computed in parallel over k-points using the functionality of the BrillouinZone applies.

If someone doesn't want parallelization they can control it through the SISL_NPROCS env variable by setting it to 1, so I didn't add any settings for it.

[zerothi]

Please remove commits changing files submodules needs a bit more care...

Also, it should already be there in the latest files commit?

[pfebrer]

Please remove commits changing files submodules needs a bit more care...

I don't understand how the submodule works exactly, sometimes things get pushed, sometimes they don't 😕

[zerothi]

Please remove commits changing files submodules needs a bit more care...

I don't understand how the submodule works exactly, sometimes things get pushed, sometimes they don't

Yeah, I generally do a commit only with the hash to ensure I do stuff correctly.

[pfebrer]

Yes, I thought of this too, but then a user will get this warning even if everything they want to do is to plot a geometry or plot the bands from a .bands file :(

[zerothi]

Yes, I thought of this too, but then a user will get this warning even if everything they want to do is to plot a geometry or plot the bands from a .bands file :(

true... hmm... Perhaps could we make it a decorator to make it portable and use the same strings, I think the warning system should catch that as "same warning"?

[pfebrer]

Which may cause unnecesary confusion and will probably be seen as a drawback for using the visualization module: "I don't use it because it says it might break my computer, I'd rather be safe" 😅

[zerothi]

Which may cause unnecesary confusion and will probably be seen as a drawback for using the visualization module: "I don't use it because it says it might break my computer, I'd rather be safe"

They will so too if it is needlessly slow if they are oversubscribing. So no good solution.... :(

But probably the decorator is the best way?

[pfebrer]

true... hmm... Perhaps could we make it a decorator to make it portable and use the same strings, I think the warning system should catch that as "same warning"?

Yes that definitely makes sense, although it is not until the point where I wrote it now that you are 100% sure the calculation is going to happen. Otherwise they will get the warning because the entry point has been attempted even if it's not going to work because there's no Hamiltonian.

[pfebrer]

Probably the warning could be issued when you do BrillouinZone.apply.renew(pool=True)? As this will also be a problem for users that do the parallellization on their own

[pfebrer]

And what about using a context manager that sets OMP_NUM_THREADS to 1 temporarily? That would work, wouldn't it?

Something similar to https://stackoverflow.com/a/34333710/11991536

[zerothi]

And what about using a context manager that sets OMP_NUM_THREADS to 1 temporarily? That would work, wouldn't it?

Something similar to https://stackoverflow.com/a/34333710/11991536

Hmm. True. I don't know of the best place... In principle BLAS is used many, many places.
I have to say I don't really think the warning will be too much of a problem. Otherwise as you say the context should take care of sharing resources the best possible way. But then again that should issue a warning telling the user that sisl is overriding this?

You could for instance imagine a user doing SISL_NPROCS=2 ; OMP_NUM_THREADS=2 with 4 cores in total. This shouldn't issue a warning since the total core-count is <=4.

If it isn't a warning but an info then it doesn't sound as severe?

[zerothi]

Perhaps we should make a 2nd round of this warning, so feel free to leave it out here. Otherwise lets keep it as is and get it in.

[pfebrer]

But then again that should issue a warning telling the user that sisl is overriding this?

I was thinking it could set the OMP_NUM_THREADS variable only if it's not defined.

Perhaps we should make a 2nd round of this warning, so feel free to leave it out here. Otherwise lets keep it as is and get it in.

I'll remove it for now then.

[pfebrer]

I've removed all spin indices for spin kinds other than unpolarized and all the tests pass, so I'd say this is ready.

[pfebrer]

I found a bug, don't merge yet!

[pfebrer]

Now it's all good! Added some more tests to make sure this bug is catched in the future :)