graphcore-research / pyscf-ipu

PySCF on IPU
https://github.com/graphcore-research/pyscf-ipu#readme
Apache License 2.0
42 stars 2 forks source link

add algebraic trick for ipu_eigh initialisation #104

Open akrzgc opened 1 year ago

akrzgc commented 1 year ago

The change:

AlexanderMath commented 1 year ago

Looks great! Minor comments: (1) some stuff wrt naming and hinting on linear algebra stuff (likely not too useful) and (2) potential performance issue with at[i].set(.) vs jax.lax.dynamic_update_slice(.) (could you try to run e.g. benzene on IPU with both, if the cycle count is +-5% my concerns are misplaced).

awf commented 1 year ago

We need a before/after graph or table to show the effect of the change, all other things being fixed

akrzgc commented 1 year ago

We need a before/after graph or table to show the effect of the change, all other things being fixed

Do you mean before/after of ipu_eigh input? Should I attach it to this review or would you like it to be plotted each time nanoDFT.py is run?