-
Notifications
You must be signed in to change notification settings - Fork 20
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
Factorization clarification question #75
Comments
I am 99% sure that the difference your are seeing is due to PyPardiso/pypardiso/scipy_aliases.py Line 44 in 0f7afd0
A matrix, which is factorized, is the same now as when it was factorized in the first place. factorize doesn't make this check and is therefore somewhat faster.
|
The ideal option, in my opinion, would be to be able to call PyPardiso/pypardiso/scipy_aliases.py Lines 71 to 77 in 0f7afd0
|
You should be able to do the following for maximum speed:
This skips all the checks etc. |
Thanks for your replies both! I'll report back with some findings |
Please be aware that |
I think I found the reason for the the performance difference that you see. I assume the brightway technosphere matrix is in csc format?
PyPardiso/pypardiso/scipy_aliases.py Line 77 in 0f7afd0
PyPardiso/pypardiso/scipy_aliases.py Lines 41 to 42 in 0f7afd0
Unfortunately my commit message from 2016 is pretty useless: 7a60c84 pardiso can deal with both PyPardiso/pypardiso/pardiso_wrapper.py Lines 219 to 227 in 0f7afd0
|
Looks like I actually wrote down the reasoning for this in #7, even with a jupyter notebook and everything 😊 |
Alright, here's some findings:
I'm not entirely sure this is correct. I think it may be the conversion to PyPardiso/pypardiso/scipy_aliases.py Lines 41 to 42 in 0f7afd0
When I convert my BW matrices to csr beforehand, I get the same speed as calling lca.decompose_technosphere() (which does this for us). solver._check_A does, however, re-do the checks for csc /csr , which doesn't seem needed when calling spsolve as we already convert to csr anyway.
I think if Brightway would set it's default format to
This adds about ~10% speed for me, while nice if you know what you're doing, perhaps it's safe that this is indeed not done for BW. Now to look ahead: Let me know if you'd like to see a PR for BW25 Chris, I'll try and so it soon-ish then. |
I also think that the best way would be if brightway uses csr format as default in combination with pypardiso.
I don't think the trade-off of 10% speed versus skipping all safety checks is worth it. If you do an LCA calculation, modify some values in the technosphere matrix and then do another LCA calculation, you might get completely wrong results without the checks in pypardiso. |
Well, partially IMO. By giving this warning and putting the factorization in an We can either automatically keep technosphere (and for consistency also biosphere?) in the correct format for the solver ( I'll close this discussion now, I think we can continue in brightway-lca/brightway2-calc#98 as this is a BW issue, not a pypardiso issue. |
I'm writing some custom BW code (can share over email, but is still to remain private for now) where I observe a ~60% speed increase (
2600200 solve/sec vs1600123 solve/sec) when I factorize beforehand.Are you certain the below is correct?
PyPardiso/pypardiso/scipy_aliases.py
Lines 73 to 74 in 0f7afd0
You may be correct that the computational/memory cost is not worth it if you only perform a few -whatever number few is-
Ax=b
solves, on the sameA
, but when I do many solves (>20k), this does seem to matter quite a lot.edit: Also note that
def factorize()
itself has conflicting information with the above.PyPardiso/pypardiso/pardiso_wrapper.py
Lines 145 to 152 in 0f7afd0
The text was updated successfully, but these errors were encountered: