Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A newly introduced validation constraint is likely too restrictive (dimensionalities max length), risking rejection of valid records as data scales.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the Cluster Materials Lux schema to move previously nested clusters and clusterPointGroups lists into separate table-style schemas, while keeping ClusterMaterial as the “main record” model and shifting certain validations to an external submission builder.
Changes:
- Introduces new table row models:
Cluster(forclusters) andClusterPointGroup(forclusterPointGroups). - Updates
FlatBandPropertiesto storelatticeDimensionalitiesandlatticeIdsas validated comma-separated strings instead of lists. - Removes nested cluster structures and associated cross-field validators from
ClusterMaterial, updating field descriptions accordingly.
File summaries
| File | Description |
|---|---|
| mpcontribs-lux/mpcontribs/lux/projects/cluster_materials/schema.py | Removes nested cluster/point-group models, updates FlatBandProperties to comma-separated string fields, and adjusts ClusterMaterial fields/descriptions accordingly. |
| mpcontribs-lux/mpcontribs/lux/projects/cluster_materials/cluster.py | Adds a table-schema Cluster model with validation for comma-separated elements and cluster invariants. |
| mpcontribs-lux/mpcontribs/lux/projects/cluster_materials/cluster_point_group.py | Adds a table-schema ClusterPointGroup model for point-group table rows. |
| mpcontribs-lux/mpcontribs/lux/projects/cluster_materials/init.py | Re-exports new public models (Cluster, ClusterPointGroup) alongside ClusterMaterial and FlatBandProperties. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Great refactor of lists to tables.
Some background on the need of an foreign key to the ClusterMaterial in Tables:
We deduplicate identical components (tables, structures) server side to save space, so if you have 2 ClusterMaterials with identical Clusters Tables, they actually share the same physical Table in our database. This causes an issue when you want to update the information for one of your ClusterMaterials: it will silently change the Table for all ClusterMaterials sharing that Table.
Adding in the unique identifier for the related ClusterMaterial causes the deduplication method to recognize each table as distinct.
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
PR is in good shape. I think after these changes we are good to go.
| def _split_comma_separated(value: str, field_name: str) -> list[str]: | ||
| """Return canonical comma-separated values or raise a validation error.""" | ||
| values = value.split(",") | ||
| if any(not item or item != item.strip() for item in values): | ||
| raise ValueError( | ||
| f"{field_name} must contain nonempty values separated by commas " | ||
| "without spaces" | ||
| ) | ||
| return values |
There was a problem hiding this comment.
Remove. Can replace call sites with just value.split(",").
I suggest tolerating white spaces:
value = [v.strip() for v in value.split(",")]
...
return ",".join(values)
The above can be used in functions that call _spit_comma_separated where ... indicates that function's specific validation logic.
There was a problem hiding this comment.
Done. The comma-separated validators now accept surrounding whitespace, validate each stripped entry, and store the canonical comma-separated representation without spaces.
| if not isinstance(cluster_material, ClusterMaterial): | ||
| raise TypeError("cluster_material must be a ClusterMaterial") | ||
| if not isinstance(clusters, pd.DataFrame): | ||
| raise TypeError("clusters must be a pandas DataFrame") | ||
| if not isinstance(cluster_groups, pd.DataFrame): | ||
| raise TypeError("cluster_groups must be a pandas DataFrame") |
There was a problem hiding this comment.
Remove: function header forces types
There was a problem hiding this comment.
Done. I removed the runtime isinstance checks and moved Cluster and ClusterPointGroup imports to the top of schema.py after eliminating the import cycle.
There was a problem hiding this comment.
We can actually remove this here. When submitting contributions, set identifier=*material.materialId
There was a problem hiding this comment.
Done. materialId has been removed from ClusterMaterial. The submission builder now uses the source material ID as the top-level contribution identifier and retains it only in the two tables as their foreign key.
Summary
This follow-up to #2142 implements the table-based representation discussed after the initial Cluster Materials schema was merged.
Clusterincluster.pyfor theclusterstableClusterPointGroupincluster_point_group.pyfor theclusterPointGroupstableclustersandclusterPointGroupsfields fromClusterMaterialCluster.elements,FlatBandProperties.latticeDimensionalities, andFlatBandProperties.latticeIdsas validated comma-separated stringsCluster.averageDistanceexplicitly in angstromsDuring submission, each contribution will contain the main
ClusterMaterialdata and two named pandas DataFrames:clustersandclusterPointGroups. Cross-component count, distance, and label checks are handled by the external submission builder because the table rows are no longer nested in the main model.Validation
clustersrows validatedclusterPointGroupsrows validatedNo data files or upload tooling are included in this pull request.