Repository navigation
Add CoRE MOF data and table schemas - #2156
Conversation
|
@bfoley12 The CoRE MOF v26.0.2 Lux schema PR is ready for your review. GitHub did not allow the fork author to add a formal review request, so I am tagging you here as requested. The PR is schema-only and includes the design questions in the description. |
c3f184c to
b6d52ea
Compare
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
In addition to comments in the PR:
- Unclear what the "Contribution" is
- All field names must be
camelCase, notsnake_case - Field names should not include units - use
client.init_columnsto set the units of the data - Lots of Literals that go unused
- Possibility for non-final contributions to be submitted
We will go through more rounds of review. There are things that will need to be refactored, but this needs to get to a more final state before that.
There was a problem hiding this comment.
This is not needed - we save structures as a Structure object internally. You can submit structures by using submit_contributions and having structures described in the submitted data.
Note: You will also need to include sites, lattice, and optionally charge fields in addition to cif.
Sites that use CifManifestRecord can reference pymatgen.Structure instead.
There was a problem hiding this comment.
It looks like this accepts Errors from a pipeline? Only successfully ran pipelines should have final data submitted.
There was a problem hiding this comment.
It looks like this accepts Errors from a pipeline? Only successfully ran pipelines should have final data submitted.
| StructureId = Annotated[ | ||
| str, | ||
| Field( | ||
| pattern=r"^(ASR|FSR|ION)-(COD|CSD|SI)-(?:\d{4}|UNKN)-\d{4}$", |
There was a problem hiding this comment.
Duplication of _STRUCTURE_ID_RE
There was a problem hiding this comment.
It looks like this accepts Errors from a pipeline? Only successfully ran pipelines should have final data submitted.
|
|
||
| parsed = parse_structure_id(structure_id) | ||
| if cif_file is not None and cif_file != f"cifs/{structure_id}.cif": | ||
| raise ValueError("cif_file must equal cifs/{structure_id}.cif") |
| PositiveSize = Annotated[ | ||
| int, | ||
| Field(gt=0, description="File size in bytes."), | ||
| ] | ||
| NonNegativeInt = Annotated[int, Field(ge=0)] | ||
| NonNegativeFloat = Annotated[float, Field(ge=0)] |
There was a problem hiding this comment.
These exist as pydantic.* - use them directly
| def _parse_dimension(value: object) -> object: | ||
| """Coerce integer CSV cells while retaining a bounded JSON integer type.""" | ||
|
|
||
| if isinstance(value, str) and value in {"0", "1", "2", "3"}: | ||
| return int(value) | ||
| return value | ||
|
|
||
|
|
||
| Dimension = Annotated[Literal[0, 1, 2, 3], BeforeValidator(_parse_dimension)] |
There was a problem hiding this comment.
Pydantic handles string coercion. Can drop the validator
| metal_element_count: NonNegativeInt = Field( | ||
| description="Number of distinct detected metal elements." | ||
| ) | ||
| metal_atom_count: NonNegativeFloat = Field( |
|
@bfoley12 Thank you for the review. I revised the schema so that one contribution represents one final CoRE MOF structure identified by a unique structureId. The previous artifact-specific schemas were replaced with one minimal CoreMofContribution using StructureType. Fields are camelCase and unit-free, optional fields default to None, and operational or non-final pipeline data were removed. The updated tests pass locally. Could you please review the revised structure? |
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
This was a major change from the last commit. Make sure you are including enough information. The information just has to be a well-structured final datapoint (with some associated data (tables, structures).
Overall good step in the right direction, just make sure you aren't leaving out additional meaningful data.
| structureId: StructureId = Field( | ||
| description="Release-preserved identifier for this CoRE MOF structure." | ||
| ) |
There was a problem hiding this comment.
Since these are your unique identifiers, they should not live in this class. when submitting contributions, you should set identifiers to your structureId
| structure: StructureType = Field( | ||
| description="Pymatgen Structure containing lattice, sites, and charge." | ||
| ) | ||
| cif: Annotated[str, StringConstraints(min_length=1)] = Field( | ||
| description="Original Crystallographic Information File text." | ||
| ) |
There was a problem hiding this comment.
We can remove these. Every Contribution carries with it a structures field. When submitting you contributions, you can also submit a Structure there.
See this Quickstart example. Except in that example, identifier would be your structureId and data is what we are describing in this model. Then, structures would be a list of pymatgen Structures.
Let me know if a further example would help!
| sourceDatabase: SourceDatabase = Field( | ||
| description="COD, CSD, or supporting-information provenance category." | ||
| ) | ||
| sourceId: Annotated[str, StringConstraints(min_length=1)] = Field( |
There was a problem hiding this comment.
Can you put a max_length on this? It helps prevent abuse. You can make it something like 1.5x the max length of the longest source ID that you have. Even better would be to more strongly enforce the format of the sourceId
| formula: str | None = Field( | ||
| default=None, | ||
| description="Chemical formula recorded for the release structure." | ||
| ) |
There was a problem hiding this comment.
This will also go in the top level of the Contribution. Here, we are just describing Contribution.data. See the Quickstart link in an earlier comment
Remove the project-specific README and schema test file from this PR. Keep all six schema and import modules and the package-level README unchanged.
100mingyu-rgb
changed the title
|
@bfoley12 I have pushed the updated CoRE MOF data and table schemas, incorporating your feedback. Could you please review the latest changes when you have a chance? Thank you for your guidance. |
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
PR is in good shape! Huge changes since the start. Few comments regarding some typing.
One overall comment: I'd caution against setting model_config to strict=True, the type conversion is pretty useful, especially with tabular data where spreadsheet software (ie. Excel) might mangle the types. Ie. opening "RAC.csv" in excel with racsFunctioNGroupDFuncI0All=1.0 transforms 1.0 -> 1. Then when saving the csv, Excel will save it as 1, which will then be rejected by StrictFloat.
Hoping we are near the end of the review cycle!
| mofidV1: str | None = Field( | ||
| default=None, description="Original MOFid v1 string, when available." | ||
| ) | ||
| mofidV2: str | None = Field( | ||
| default=None, description="Original MOFid v2 string, when available." | ||
| ) |
There was a problem hiding this comment.
Can a MOF have both a V1 and V2 id? And can you add a standard format for these?
There was a problem hiding this comment.
Yes, a MOF can have both a V1 and a V2 ID. MOFid V2 is generated by post-processing the V1 calculation outputs, incorporating node composition and type information and converting linker representations from SMILES to SELFIES. The two IDs therefore represent the same MOF in different formats, and we retain both when available.
| metalElements: str | None = Field( | ||
| default=None, description="Semicolon-separated metal symbols." | ||
| ) |
There was a problem hiding this comment.
Add a field_validator that uses pymatgen.core.Element.is_metal() on each element in list to verfy.
| mofClassifierStatus: Literal["PASS", "FAIL"] = Field( | ||
| description="Completed MOFClassifier vote." | ||
| ) | ||
| mofCheckerStatus: Literal["PASS", "FAIL"] = Field( | ||
| description="Completed MOFChecker vote." | ||
| ) | ||
| chenManzStatus: Literal["PASS", "FAIL"] = Field( | ||
| description="Completed Chen-Manz vote." | ||
| ) | ||
| mosaecStatus: Literal["PASS", "FAIL"] = Field(description="Completed MOSAEC vote.") | ||
| setcGatStatus: Literal["PASS", "FAIL"] = Field( | ||
| description="Completed SETC-GAT vote." | ||
| ) |
There was a problem hiding this comment.
Make Pass/Fail an Enum since it is repeated a lot.
class CheckerStatus(StrEnum):
PASS = "PASS"
FAIL = "FAIL"
| consensus3: Literal["CR", "NCR", "AMBIGUOUS"] = Field( | ||
| description="Recorded agreement of MOFClassifier, MOFChecker, and Chen-Manz." | ||
| ) | ||
| consensus4: Literal["CR", "NCR", "AMBIGUOUS"] = Field( | ||
| description="Recorded agreement of those three checkers and MOSAEC." | ||
| ) | ||
| consensus5: Literal["CR", "NCR", "AMBIGUOUS"] = Field( | ||
| description="Recorded agreement of all five checkers." | ||
| ) |
There was a problem hiding this comment.
Similar to above, make a StrEnum.
| from pydantic import BaseModel, ConfigDict, StrictFloat | ||
|
|
||
|
|
||
| class RacFeatures(BaseModel): |
There was a problem hiding this comment.
Can you explain the format of this to me? A single contribution has this as a table, each field is a column name in that table, and a contribution has many of these, is that correct?
There was a problem hiding this comment.
Each Contribution optionally includes one rac5Features Table with exactly one row and 264 columns, one column per descriptor. The columns are features of a single structure, rather than separate records.
These revised autocorrelation (RAC) descriptors encode atomic properties and connectivity for use as machine-learning features. The maximum graph depth is 5, covering bond distances from 0 to 5. If no valid RAC result is available, we omit only the rac5Features Table.
| racsFunctionGroupDFuncI0All: StrictFloat | ||
| racsFunctionGroupDFuncI1All: StrictFloat | ||
| racsFunctionGroupDFuncI2All: StrictFloat |
There was a problem hiding this comment.
For all of the fields in this class, I would suggest making them regular floats. StrictFloat will prevent converting from int to float, and some csv software might convert floats to ints if they end in .0 ie. 1.0 -> 1. I would remove strict=True from model_config for the same reason. Unless there is a specific reason that you made them strict, in which case it is fine to leave it.
| class Topology(BaseModel): | ||
| """One subnet from a successful, available parent topology result.""" | ||
|
|
||
| model_config = ConfigDict(extra="forbid", allow_inf_nan=False, strict=True) |
There was a problem hiding this comment.
Similar to rac_features (and just generally), I'd only set model_config to strict if you are certain floats or bools will not be entering int fields.
|
@bfoley12 Thank you for taking the time to provide feedback despite your busy schedule. I have updated the schemas to address your comments. |
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Nice. Last change I found here.
| hasOpenMetalSites: bool | None = Field( | ||
| default=None, | ||
| description="Whether open metal sites were detected; false is a valid successful result.", | ||
| ) | ||
| openMetalSiteCount: NonNegativeInt | None = Field( | ||
| default=None, description="Number of detected open metal sites, dimensionless." | ||
| ) |
There was a problem hiding this comment.
I assume when hasOpenMetalSites == True that openMetalSiteCount != None? If that is true, then hasOpenMetalSites would be redundant and we don't need to include it.
There was a problem hiding this comment.
I confirmed that the field was redundant and removed it.
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Saw some inconsistencies in the Readme with the models. Can you add the validators I specified?
| consensus4: ConsensusStatus = Field( | ||
| description="Recorded agreement of those three checkers and MOSAEC." |
There was a problem hiding this comment.
Can you be explicit in which checkers (consensus3 + MOSAEC)
| | `geometry.vpov` | VPOV | `poavVolFrac + ponavVolFrac` | dimensionless | | ||
| | `geometry.gpov` | GPOV | `vpov / Zeo.density` | cm^3/g | | ||
|
|
There was a problem hiding this comment.
Either add validation for this, or make vpov and gpov properties
| - Topology: include summary details and subnet rows only for a successful, | ||
| available parent result. ERROR/PARTIAL parents contribute only | ||
| `data.topology.topologyAvailable = False`. |
There was a problem hiding this comment.
Add validation on the model
|
|
||
| OMS presence is derived as `openMetalSiteCount > 0` when `omsAvailable` is | ||
| true. The redundant `hasOpenMetalSites` column is not included. A zero count | ||
| means no detected OMS; an unavailable result remains missing, rather than zero. |
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Saw some inconsistencies in the Readme with the models. Can you add the validators I specified?
|
I've addressed your feedback. Thank you! |
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Few more comments for tightening fields
| def gpov(self) -> float | None: | ||
| if self.vpov is None or self.density is None: | ||
| return None | ||
| return self.vpov / self.density |
There was a problem hiding this comment.
I see you made these computed_fields. Since you want to keep them fields (that's fine, it's coming from your data), just add a validator for them that checks that they are close to their calculations. Computed fields with extra=forbid don't deserialize and raise an error (extra=forbid is correct here)
| metalElements: str | None = Field( | ||
| default=None, | ||
| description=( | ||
| "Semicolon-separated element symbols, each classified as a metal " | ||
| "by pymatgen." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Add a reasonable cap on length (you don't have to be strict - I don't want you to have to update this as your experiments grow). Enforce semi-colon separated list format (could use a type so it can be used for metalloidElements too)
| metalloidElements: str | None = Field( | ||
| default=None, description="Semicolon-separated metalloid symbols." | ||
| ) |
There was a problem hiding this comment.
See comment on metalElements
| dif: float | None = Field( | ||
| default=None, | ||
| description=( | ||
| "MOFSimplify Dif: maximum included-sphere diameter along the " | ||
| "free-sphere path, angstrom; source sign policy is handled upstream." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Sounds like this is a diameter, can you specify >(=)0?
| singleNodeTopologyKey: str = Field( | ||
| min_length=1, | ||
| description="Nonblank SingleNodes topology key from the successful source result.", | ||
| ) | ||
| singleNodeTopologyName: str | None = Field( | ||
| default=None, | ||
| description="Recorded SingleNodes topology name; original empty strings are retained.", | ||
| ) | ||
| singleNodeTopologicalGenome: str | None = Field( | ||
| default=None, | ||
| description="Recorded SingleNodes topological genome; original empty strings are retained.", | ||
| ) | ||
| allNodeDimension: int = Field( | ||
| ge=0, le=3, description="AllNodes network dimension, 0 to 3, dimensionless." | ||
| ) | ||
| allNodeTopologyKey: str = Field( | ||
| min_length=1, | ||
| description="Nonblank AllNodes topology key from the successful source result.", | ||
| ) | ||
| allNodeTopologyName: str | None = Field( | ||
| default=None, | ||
| description="Recorded AllNodes topology name; original empty strings are retained.", | ||
| ) | ||
| allNodeTopologicalGenome: str | None = Field( | ||
| default=None, | ||
| description="Recorded AllNodes topological genome; original empty strings are retained.", | ||
| ) |
There was a problem hiding this comment.
If possible, specify upper bound of str length. If there is a common pattern, please specify (otherwise, just use the max length) - feel free to be loose with the cap so you don't have to make a PR for each new experiment
|
Thank you for your time and helpful feedback. validation against the full dataset and
boundary cases passed. Thank you again.
|
bfoley12
left a comment
bfoley12
left a comment
There was a problem hiding this comment.
Great. Very happy. Last 2 comments I can find.
|
|
||
| model_config = ConfigDict(extra="forbid", allow_inf_nan=False) | ||
|
|
||
| releaseVersion: str = Field(min_length=1, description="CoRE dataset version.") |
There was a problem hiding this comment.
Do you have regex for this? If so add
|
Dear Brendan,
Thank you for your helpful feedback. I have addressed both comments:
1. Added a validator to MetalData to ensure that metalElementCount
matches the number of unique symbols in metalElements. Duplicate symbols
are rejected, and a missing or None list is allowed only when the count
is zero.
2. Added a regex to ReleaseData.releaseVersion to enforce the
v<number>.<number>.<number> format, such as v26.0.2.
Thank you again.
On Mon, 02 Nov 2026 16:14:02 -0800, Brendan Foley ***@***.***> wrote:
@bfoley12 requested changes on this pull request. Great. Very happy. Last 2
comments I can find. > + +class MetalData(BaseModel): + """Metal and
metalloid annotations.""" + + model_config = ConfigDict(extra="forbid",
allow_inf_nan=False) + + metalElements: ElementSymbolList | None = Field( +
default=None, + description=( + "Semicolon-separated element symbols, each
classified as a metal " + "by pymatgen." + ), + ) + metalElementCount:
NonNegativeInt = Field( + description="Number of distinct metal elements."
+ ) I assume `metalElementCount == len(metalElements.split(";")`? If that
is true, can you add a validator? > +from pymatgen.core import Element + +
+def reject_boolean_number(value: Any) -> Any: + """Reject boolean scalars
without disabling ordinary numeric coercion.""" + if isinstance(value,
(bool, NumpyBool)): + raise ValueError("A boolean is not a scientific count
or magnitude") + return value + + +class ReleaseData(BaseModel): +
"""Dataset version information.""" + + model_config =
ConfigDict(extra="forbid", allow_inf_nan=False) + + releaseVersion: str =
Field(min_length=1, description="CoRE dataset version.") Do you have regex
for this? If so add -- Reply to this email directly or view it on GitHub:
#2156 (review)
You are receiving this because you authored the thread. Message ID:
***@***.***>
|
|
@100mingyu-rgb Great thanks! Last question: how large are the topology tables in terms of bytes? <16MB? |
|
Among the final upload candidates, the largest individual topology Table is
23,292 bytes, and all topology Tables are below 16 MB.
On Tue, 03 Nov 2026 13:09:02 -0800, Brendan Foley ***@***.***> wrote:
bfoley12 left a comment (materialsproject/MPContribs#2156) @100mingyu-rgb
Great thanks! Last question: how large are the topology tables in terms of
bytes? <16MB? -- Reply to this email directly or view it on GitHub:
#2156 (comment)
You are receiving this because you were mentioned. Message ID:
***@***.***>
|
This PR defines Pydantic schemas for CoRE MOF. Each Contribution represents one CoRE MOF structure.
data.pyDataContribution.data: nine groups with 46 input fields (15 required, 31 optional), including two automatically calculated volume fieldszeo_features.pyZeoFeatureszeoFeaturesTable, with 26 columnstopology.pyTopologytopologySubnetsTable, with 10 columnsrac_features.pyRacFeaturesrac5FeaturesTable with one row and 264 numeric RAC descriptors, using a maximum graph depth of 5Identifiers, formulas, and structures use native MPContribs components. The data model contains source and reference information, MOFid, crystallographic and metal information, checker results, topology summaries, and MOFSimplify geometric descriptors with density as an additional calculation input.
GeometryDatacalculates missingvpovandgpovvalues and validates supplied values against their calculation inputs. Model validators check metal element lists and counts, checker consensus, topology summaries and rows, and OMS availability and count. The exportedvalidate_topology(summary, rows)function checks relationships between the topology summary and its Table.Field names use camelCase. Optional fields default to
None, numeric values must be finite, and ordinary Pydantic type conversion remains enabled. Boolean values are rejected as numeric inputs.An optional CCDC search URL is restricted to CSD sources and must match the source refcode.
Topology keys and names have bounded lengths; genomes additionally validate a space-separated integer representation.
Release versions follow the
v<number>.<number>.<number>format, such asv26.0.2.