Ensemble: opt-in linear-memory q-space mode, plus four fixes on the Fourier path - #428
Ensemble: opt-in linear-memory q-space mode, plus four fixes on the Fourier path#428SorBalda wants to merge 2 commits into
Conversation
…ourier path Opt-in flag Ensemble(..., qspace_light=True), default False, so nothing changes unless it is asked for. In light mode the dense (3N, 3N) supercell polarization vectors are never materialized -- not even at construction -- and reweighting goes through update_weights_fourier, which works per q point. This is what makes the q-space TDSCHA path linear in memory instead of quadratic; it needs CellConstructor's DiagonalizeSupercell(q_only=True). Because the polarization vectors do not exist in this mode, the routines that genuinely need them (get_free_energy, get_free_energy_hessian_dev) now fail with an explicit message instead of an AttributeError on None deep inside CellConstructor. refresh_qspace_caches_from_real_space() is public. After a real-space update_weights the q-space caches are stale, and the consumer (tdscha) had to call a private method and then reach into Ensemble.__dict__ to raise the coherence marker. The public method raises the marker itself, so the invariant cannot drift and no caller needs private state. The private name remains as a deprecated alias. Four bugs fixed on the Fourier path, all independent of the flag above: * init(): "self.N = self.structures" assigned the list itself instead of its length, so the following [None] * self.N and np.tile(..., (self.N,1,1)) could not work once that branch was taken. * the Fourier sscha_energies used forces_qspace (the ab initio forces) where it needs sscha_forces_qspace. The same expression is written correctly in update_weights_fourier; the two sites now agree. * init_from_structures() left the Fourier energies in Ry/Bohr (missing BOHR_TO_ANGSTROM) and with the wrong overall sign, again unlike the equivalent block elsewhere in the file. update_weights_fourier also rejects, instead of silently mixing, a new dynamical matrix whose q points are reordered or whose unit cell differs: the Fourier update contracts dynq and u_disps_qspace index by index, and r_lat / q_grid are fixed at generation. Tested by tests/test_qspace_light (11 tests), which check that light and standard ensembles agree and that the light path refuses what it cannot do.
…peError The light mode calls DiagonalizeSupercell(q_only=True), which only newer CellConstructor releases provide. Against an older one the flag failed with 'DiagonalizeSupercell() got an unexpected keyword argument q_only', raised from inside __setattr__ and pointing at neither the flag nor the requirement. A module-level probe now reports the capability, so the constructor raises an explanatory error, and tests/test_qspace_light skips instead of failing when the installed CellConstructor cannot support it. Nothing else changes: the default path never touches q_only.
mesonepigreco
left a comment
mesonepigreco
left a comment
There was a problem hiding this comment.
The architecture of this PR is actually terrible. Is an hack, not a releasable code.
The API of Ensemble MUST BE STABLE, regardless of the version of other packages like cellconstructor.
What can be done is to use a top-module flag that check wether cellconstructor is at least version 1.7, and raises an error of incompatibility otherwise (CC new version must also be flagged as 1.7, we will merge cellconstructor PR first, so after that point, this version can see the 1.7 CC version and correctly install and work).
Then, all hacks of hassatr dict , and so must be avoided at all costs!!!
Moreover, the get_free_energy module does not need at all the sc vectors, as it might work also with q space polarization. This will allow it to run faster. The only problem is in the get_free_energy_hessian_dev, at which they could be computed and initialized live at the beginning of that function. Therefore, the q_only can become the only implemented way in the ensemble and everything is much cleaner (so remove all the flags). We must avoid this branching of kind of ensembles. Only one way should be implemented. Moreover, we can remove the supercell pathways, as this was obsolete since version 1.4, at 1.7 we could make the julia path the only working environment (as there is a massive difference in both performances and memory allocation).
Moreover, some of the edits are on dead code. We could probably remove dead code if identified.
| # only newer CellConstructor releases provide. Probe it once, so that asking | ||
| # for the flag against an older one gives an explanatory error instead of a | ||
| # TypeError raised from inside __setattr__. | ||
| import inspect as _inspect |
There was a problem hiding this comment.
Same as the other. Forbidden to use inspect. instead constrain version to all 1.7
Eventually, you can inspect if cellconstructor version is < 1.7 and raise an exception. The PR of cellconstructor and this one needs to move the version to 1.7.
| self.current_pols = self.pols_0.copy() | ||
| self.w_q_current = self.w_q_0.copy() | ||
| self.pols_q_current = self.pols_q_0.copy() | ||
| if self.__dict__.get("qspace_light", False): |
There was a problem hiding this comment.
This has to be avoided at any cost! Use standard
if not self.qspace_light:
If it raises an error, then the code architecture is implemented wrongly. We cannot accept these code inside the main branch.
There was a problem hiding this comment.
Moreover this is very hacky. If you need to initialize something, do it at module init level! Moreover, Ensemble API must be stable and cannot depend on version of other libraries installed. So every element must be defined in the init for all path. Allocation is a different thing and can be done in a separate allocate() method that performs allocations and initialization (maybe called by the init). This is the clean way to implement things.
| self.w_q_current = self.w_q_0.copy() | ||
| self.pols_q_current = self.pols_q_0.copy() | ||
| if self.__dict__.get("qspace_light", False): | ||
| if not _CC_HAS_Q_ONLY: |
There was a problem hiding this comment.
Here a version control on cellconstructor is sufficient. Maybe use a global flag on the top of the module for version rather for having individual attributes.
| # The caches have been rebuilt from scratch: force the q-space | ||
| # consumers (QSpaceLanczos) to re-derive them from the real-space | ||
| # arrays rather than trusting a stale provenance marker. | ||
| self.__dict__["_last_weight_update_fourier"] = False |
There was a problem hiding this comment.
NO! Super hacky! This needs to be completely rewritten
| self.__dict__["_last_weight_update_fourier"] = False | ||
|
|
||
|
|
||
| def _refresh_qspace_caches_from_real_space(self): |
There was a problem hiding this comment.
Why is it deprecated and introduced in this same PR? clean it
|
|
||
| w_original = self.w_0.copy() | ||
| pols_original = self.pols_0.copy() | ||
| if not self.__dict__.get("qspace_light", False): |
| # Exclude translations | ||
| if not self.ignore_small_w: | ||
| trans_original = super_struct0.get_asr_modes(pols_original) | ||
| if self.__dict__.get("qspace_light", False): |
|
|
||
| if not self.ignore_small_w: | ||
| trans_mask = super_structure.get_asr_modes(pols) | ||
| if self.__dict__.get("qspace_light", False): |
| Since it is the most time consuming part, it can be safely avoided. | ||
| """ | ||
|
|
||
| if getattr(self, "qspace_light", False): |
| #print( "Time elapsed to update weights the sscha energies, forces and displacements:", t1 - t3, "s") | ||
| else: | ||
| self.current_dyn = new_dynamical_matrix.Copy() | ||
| self.__dict__["_last_weight_update_fourier"] = False |
2 participants
Ensemble: opt-in linear-memory q-space mode, plus four fixes on the Fourier path
Part of a three-repository series that makes the q-space TDSCHA path linear in
memory instead of quadratic. The four bug fixes at the end are independent of
the new flag and stand on their own.
Ensemble(..., qspace_light=True)Opt-in, default
False: nothing changes unless it is asked for. In light modethe dense
(3N, 3N)supercell polarization vectors are never materialized — noteven at construction — and reweighting goes through
update_weights_fourier,which works per q point. This is what makes the q-space TDSCHA path linear in
the supercell size; it needs
DiagonalizeSupercell(q_only=True)from theCellConstructor PR above.
Because the polarization vectors do not exist in this mode, the routines that
genuinely need them (
get_free_energy,get_free_energy_hessian_dev) now failwith an explicit message naming the flag, instead of an
AttributeErroronNoneraised deep inside CellConstructor.refresh_qspace_caches_from_real_space()is publicAfter a real-space
update_weightsthe q-space caches are stale. The consumer(tdscha) had to call a private method and then reach into
Ensemble.__dict__toraise the coherence marker. The public method raises the marker itself, so the
invariant cannot drift and no caller touches private state. The private name
remains as a deprecated alias.
Four bugs on the Fourier path
These are independent of
qspace_lightand fire on any ensemble:init():self.N = self.structuresassigned the list itself instead ofits length, so the following
[None] * self.Nandnp.tile(..., (self.N,1,1))could not work once that branch was taken.sscha_energiesusedforces_qspace(the ab initio forces) whereit needs
sscha_forces_qspace. The same expression is written correctly inupdate_weights_fourier; the two sites now agree.init_from_structures()left the Fourier energies in Ry/Bohr (missingBOHR_TO_ANGSTROM) and with the wrong overall sign — again unlike theequivalent block elsewhere in the same file.
update_weights_fouriernow also rejects, instead of silently mixing, a newdynamical matrix whose q points are reordered or whose unit cell differs: the
Fourier update contracts
dynqandu_disps_qspaceindex by index, andr_lat/q_gridare fixed at generation time.Testing.
tests/test_qspace_light/(11 tests) checks that light and standardensembles agree and that the light path refuses what it cannot do. The full
suite passes with the merge of current
master.