CederGroupHub / smol

Statistical Mechanics on Lattices
https://cedergrouphub.github.io/smol/
Other
65 stars 14 forks source link

Reloading clustersubspace using orthonormal, does not keep orthonormalization #89

Closed lbluque closed 4 years ago

lbluque commented 4 years ago

Orthonormalizing the site basis sets in a clusterspace is not recorded when saving the objects as mson dicts, so when reloaded the basis sets are not orthonormal.

Expected Behavior

The state of a cluster subspace should be saved completely to faithfully recreate the exact same object from a dictionary.

Current Behavior

Since the SiteBasis class is not MSONable, the exact state of the basis sets is not completely saved when saving a ClusterSubspace.

Possible Solution

Either make the SiteBasis class MSOnable and save its full state, or create boolean flags as attributes to keep track of changes, specifically orthonormalizing. As a quick and dirty fix users can simply "remember" if the were using an orthonormal set and then set the basis accordingly (this should NOT be a long term solution and the issue should be fixed in the source code asap):

subspace = ClusterSubpsace.from_dict(d)
subspace.change_site_bases('same basis as before', orthormal=True)

Steps to Reproduce

  1. Create a ClusterSubspace with orthonormal=True
  2. Save it as dict
  3. Load it from dict
    
    from smol.cofe import ClusterSubspace
    subspace = ClusterSubspace.from_cutoffs(prim, {2: 6, 3: 4}, basis='sinusoid', orthonormal=True)
    subspace1 = ClusterSubspace.from_dict(subspace.as_dict())

assert subspace.basis_orthonormal # this is good assert subspace1.basis_orthonormal # this is not good!



## Possible Implementation
<!--- Not obligatory, but suggest an idea for implementing addition or change -->
Make the `SiteBasis` class `MSONable` such that it saves the exact values of all the bases arrays and therefore faithfully saves any changes done to it. (Better solution imo)
Add boolean attributes to record changes in state, ie, orbit._basis_orthonormal, and save in in dict, such that when the object are recreated the basis sets are properly normalized.

* Add a check of basis sets in unit tests checking as_dict/from_dict of `ClusterSubspaces`
lbluque commented 4 years ago

@tchen0965 @juliayang @zjadidi please have a look at this and make sure it has not affected you.

I am working on fixing this asap.

juliayang commented 4 years ago

@lbluque looks like the second assertion for subspace1 failed :(

Let us know if we can help you anything!

lbluque commented 4 years ago

Thanks @juliayang! That's what I suspected. I already committed a fix to my fork, but need to write some tests to check this going forward.

For now you can force the second assert statement and result consistency using the temporary fix I suggested above.

lbluque commented 4 years ago

fix added in #90