Merge pull request #73 from mikibonacci/roundtrips

This fixes #71

Author: mikibonacci

docs/source/how_to/creation_mutation.md

@@ -270,20 +270,19 @@ Magnetic moments:
 [[0.  0.  2.5]]
 ```
 
-### From Legacy AiiDA StructureData
+### Backward compatibility: from Legacy orm.StructureData
 
-Migrate from the legacy AiiDA StructureData:
+It is possible to migrate from the old `orm.StructureData` to the new `aiida-atomistic` `StructureData`:
 
 ```python
-from aiida import orm
-from aiida_atomistic.data.structure import StructureData
+from aiida.orm import StructureData as LegacyStructureData
+from aiida_atomistic.data.structure.utils_orm import from_legacy_to_atomistic
 
-# Load legacy structure
-legacy_structure = orm.StructureData()
-legacy_structure.append_atom(symbols='C', position=(0.0, 0.0, 0.0))
+legacy = LegacyStructureData(cell=[[3.0, 0.0, 0.0], [0.0, 3.0, 0.0], [0.0, 0.0, 3.0]])
+legacy.append_atom(symbols='H', position=[0.0, 0.0, 0.0], mass=1.008, name='H1')
+legacy.append_atom(symbols='O', position=[0.0, 0.0, 1.0], mass=15.999, name='O1')
 
-# Convert to new format
-new_structure = legacy_structure.to_atomistic()
+s = from_legacy_to_atomistic(legacy_structure=legacy, metadata={'store_provenance': True}) # no need to define metadata, default is already True.
 ```
 
 ## Modifying Structures
@@ -292,7 +291,7 @@ new_structure = legacy_structure.to_atomistic()
 `StructureData` is **immutable** and cannot be modified. Use `StructureBuilder` for modifications.
 :::
 
-### Creating a Mutable Structure
+### Creating a Mutable Structure: the `StructureBuilder`
 
 ```python
 from aiida_atomistic.data.structure import StructureBuilder
@@ -436,17 +435,20 @@ print(f"Magmoms after removal: {mutable.properties.magmoms}")  # None
 - See the full list of properties in the [Site API documentation](../reference/api/auto/aiida_atomistic/data/structure/site/index.rst)
 :::
 
-### Converting Between Mutable and Immutable
+## Conversion Between `StructureBuilder` and `StructureData` (and viceversa)
+
+The `StructureBuilder` object is a pure python class, any instance of it needs to be converted into the `StructureData` before being used in an AiiDA process.
+It is possible to seamlessly convert between `StructureBuilder` and `StructureData` (and viceversa) using the defined `to_*` and `from_*` methods:
 
 ```python
-# Mutable → Immutable (for storage in AiiDA)
-immutable = StructureData(**mutable.to_dict())
+# StructureBuilder → StructureData (for storage in AiiDA)
+structuredata = structurebuilder.to_aiida()
+structuredata = StructureData.from_builder(structurebuilder)
 
 # Immutable → Mutable (for editing)
-mutable_copy = StructureBuilder(**immutable.to_dict())
+structurebuilder = structuredata.to_builder()
+structurebuilder = StructureBuilder.from_aiida(structuredata)
 
-# Or use get_value()
-mutable_copy2 = immutable.get_value()
 ```
 
 ## Generating Kinds Automatically

docs/source/how_to/kinds.md

@@ -171,7 +171,7 @@ By default, `StructureData` validates kinds on creation:
 
 ```python
 # This validates that sites with the same kind_name have identical properties
-structure = StructureData(**structure_dict, validate_kinds=True)  # Default
+structure = StructureData(**structure_dict, validate_kinds=True)  # Default is False
 ```
 
 If validation fails, you'll get an error:
@@ -211,7 +211,7 @@ In some cases, you may want to skip validation:
 
 ```python
 # Skip validation on creation
-structure = StructureData(**structure_dict, validate_kinds=False)
+structure = StructureData(**structure_dict, validate_kinds=False) # Default behavior
 ```
 
 :::{warning}

pyproject.toml

@@ -26,7 +26,8 @@ dependencies = [
     "voluptuous",
     "ase",
     "pymatgen>=2022.1.20",
-    "pydantic>=2.6"
+    "pydantic>=2.6",
+    "scipy>=1.10",
 ]
 
 [project.urls]

src/aiida_atomistic/data/structure/getter_mixin.py

@@ -13,7 +13,7 @@
     HubbardGetterMixin,
 )
 
-from aiida_atomistic.data.structure.utils import classify_site_kinds, check_kinds_match, efficient_copy
+from aiida_atomistic.data.structure.utils import classify_site_kinds, check_kinds_match
 
 try:
     import ase  # noqa: F401
@@ -388,16 +388,15 @@ def validate_kinds(self,):
             raise ValueError("The kinds defined in the structure do not match the generated kinds from the sites. Please run the 'generate_kinds' method to see the expected kinds.")
 
     # TO methods:
-    def to_dict(self, exclude_kinds=False):
+    def to_dict(self):
             """
             Convert the structure to a dictionary representation.
 
-            :param detect_kinds: Whether to detect and include the kinds of the structure.
-            :type detect_kinds: bool, optional
             :return: The structure as a dictionary.
             :rtype: dict
             """
-            dict_repr = efficient_copy(self.properties.model_dump(exclude_unset=True, exclude_none=True, warnings=False, exclude={'kinds'} if exclude_kinds else {}))
+            exclude = set(self.properties.model_computed_fields.keys())
+            dict_repr = copy.deepcopy(self.properties.model_dump(exclude_unset=True, exclude_none=True, warnings=False, exclude=exclude))
 
             return dict_repr
 

src/aiida_atomistic/data/structure/models.py

@@ -64,6 +64,7 @@ class StructureBaseModel(BaseModel):
         from_attributes=True,
         frozen=False,
         arbitrary_types_allowed=True,
+        extra='forbid',
         #validate_assignment=True
     )
 
@@ -116,9 +117,9 @@ def validate_sites(cls, v):
 
         if v is None:
             return v
-        else:
-            # test if they can be converted to Site
-            sites = [Site.model_validate(site) if not isinstance(site, Site) else site for site in v]
+        # else:
+        #     # test if they can be converted to Site
+        #     sites = [Site.model_validate(site) if not isinstance(site, Site) else site for site in v]
 
         _check_valid_sites(v)
 
@@ -291,20 +292,34 @@ def kinds(self) -> list[Kind]:
             #raise ValueError("Kind names must be defined to access kinds.")
             return None
 
+        # Mapping of kind_name -> site indices
+        kind_to_indices = defaultdict(list)
+        for i, name in enumerate(self.kind_names):
+            kind_to_indices[name].append(i)
+
+        positions_array = self.positions
+
         kinds_list = []
-        kind_name_set = set(self.kind_names)
-        for idx, site in enumerate(self.sites):
+        seen_kinds = set()
+
+        for site in self.sites:
             kind_name = site.kind_name if site.kind_name else site.symbol
-            if kind_name in kind_name_set:
-                site_indices = [i for i, name in enumerate(self.kind_names) if name == kind_name]
-                positions=np.array([self.positions[i] for i in site_indices])
-                kind = Kind(
-                    **site.model_dump(exclude={'position'}),
-                    site_indices=site_indices,
-                    positions=positions,
-                )
-                kinds_list.append(kind)
-                kind_name_set.remove(kind_name)  # Ensure we don't add the same kind multiple
+
+            # Skip if we've already processed this kind
+            if kind_name in seen_kinds:
+                continue
+
+            seen_kinds.add(kind_name)
+            site_indices = kind_to_indices[kind_name]
+            positions = positions_array[site_indices]
+
+            kind = Kind(
+                **site.model_dump(exclude={'position','kind_name'}),
+                site_indices=site_indices,
+                positions=positions,
+                kind_name=kind_name
+            )
+            kinds_list.append(kind)
 
         return FrozenList(kinds_list)
 

src/aiida_atomistic/data/structure/site.py

@@ -168,8 +168,9 @@ def check_minimal_requirements(cls, data):
         #if "kind_name" not in data:
         #    data["kind_name"] = data["symbol"]
 
-        for prop in data.keys():
-            if cls._mutable:
+
+        if cls._mutable:
+            for prop in data.keys():
                 data[prop] = freeze_nested(data[prop])
 
         return data

src/aiida_atomistic/data/structure/structure.py

@@ -18,7 +18,7 @@ class StructureData(Data, GetterMixin):
     _mutable = False
     _model = ImmutableStructureModel
 
-    def __init__(self, validate_kinds=True, sites:list[dict]=None, kinds:list[dict]=None, **kwargs):
+    def __init__(self, sites:list[dict]=None, kinds:list[dict]=None, **kwargs):
 
         if sites is not None and kinds is not None:
             warnings.warn("Provided both `sites` and `kinds` information. Dropping the `sites` information and using only `kinds`.")
@@ -29,8 +29,8 @@ def __init__(self, validate_kinds=True, sites:list[dict]=None, kinds:list[dict]=
         self._properties = self._model(sites=sites, **kwargs)
         super().__init__()
 
-        if validate_kinds and self.kinds is not None:
-            self.validate_kinds()
+        #if validate_kinds:
+        #    if self.kinds is not None: self.validate_kinds()
 
         attributes = self.properties.model_dump(exclude_unset=True, exclude_none=True, warnings=False)
         if self.properties.kind_names is not None:
@@ -58,15 +58,13 @@ def properties(self):
             return self._properties
 
     @classmethod
-    def from_builder(cls, mutable_structure, validate_kinds=True):
-        if not isinstance(mutable_structure, StructureBuilder):
-            raise ValueError(f"Input structure should be of type StructureBuilder, not {type(mutable_structure)}")
-        return cls(validate_kinds=validate_kinds, **mutable_structure.to_dict(exclude_kinds=True))
+    def from_builder(cls, builder: 'StructureBuilder'):
+        from aiida_atomistic.data.structure.structure import StructureBuilder
+        if not isinstance(builder, StructureBuilder):
+            raise ValueError(f"Input builder should be of type StructureBuilder, not {type(builder)}")
+        return cls(**builder.to_dict())
 
-    def to_mutable(self,):
-        return StructureBuilder(**self.to_dict())
-
-    def get_value(self):
+    def to_builder(self) -> 'StructureBuilder':
         return StructureBuilder(**self.to_dict())
 
     def __repr__(self) -> str:
@@ -89,7 +87,7 @@ class StructureBuilder(GetterMixin, SetterMixin):
     _mutable = True
     _model = MutableStructureModel
 
-    def __init__(self, validate_kinds=True, sites:list[dict]=None, kinds:list[dict]=None, **kwargs):
+    def __init__(self, sites:list[dict]=None, kinds:list[dict]=None, **kwargs):
 
         if sites is not None and kinds is not None:
             warnings.warn("Provided both `sites` and `kinds` information. Dropping the `sites` information and using only `kinds`.")
@@ -104,6 +102,15 @@ def __init__(self, validate_kinds=True, sites:list[dict]=None, kinds:list[dict]=
     def properties(self):
         return self._properties
 
+    @classmethod
+    def from_aiida(cls, aiida: 'StructureData'):
+        if not isinstance(aiida, StructureBuilder):
+            raise ValueError(f"Input aiida should be of type StructureBuilder, not {type(aiida)}")
+        return cls(**aiida.to_dict())
+
+    def to_aiida(self) -> 'StructureData':
+        return StructureData(**self.to_dict())
+
     def __repr__(self) -> str:
         """Return a concise string representation of the structure."""
         prop_repr_str = self.properties.__repr__()