feature/SOF-7990 fix: PointsGridDataProvider derives gridMetricValue - #162
feature/SOF-7990 fix: PointsGridDataProvider derives gridMetricValue #162VsevolodX wants to merge 6 commits into
Conversation
…given PointsGridDataProvider(dimensions=X, isEdited=True) -- the notebook path -- never supplied gridMetricValue, so it defaulted to the DEFAULT_KPPRA sentinel (-1) and got persisted as if it were a deliberate, edited value. That sentinel then permanently locks the k-grid from further editing in the UI: the form's own schema requires gridMetricValue >= 1 for KPPRA, RJSF resends the full form state on every edit, and -1 rides along on every one of them, failing validation every time with no error shown. Mirrors PointsGridFormDataProvider.setData() in JS, which already derives the metric from dimensions whenever a user sets them manually. An explicitly passed gridMetricValue is still respected; the untouched default path (no dimensions, no metric) is unaffected.
exabyte-io-bot
left a comment
There was a problem hiding this comment.
2 blocker(s) to resolve before this merges, across 5 finding(s).
Batched findings
- nit
tests/py/context/test_points_grid_data_provider.py:123— (AGENTS.md 5.1) A couple of small nits, non-blocking: here and on line 132, let's spell out the math (e.g.4 * 4 * 4and4 * 4 * 4 * 2) instead of hardcoding the magic numbers64and128. It makes the test act as self-documenting proof of the logic. Also, incalculate_grid_metric, theunitsargument is completely unused — let's remove it if the base interface allows it. Happy to approve once those are in.
Generated from Timur Bazhirov's review corpus (12,340 of his own past comments). Severity follows AGENTS-code-review-tb.md; confidence is the model's own estimate.
| shifts: List[float] = Field(default_factory=lambda: [0.0, 0.0, 0.0]) | ||
| gridMetricType: GridMetricType = Field(default=GridMetricType.KPPRA) | ||
| gridMetricValue: float = Field(default=DEFAULT_KPPRA) | ||
| n_atoms: int = Field(default=1, exclude=True) |
There was a problem hiding this comment.
(TB-NAME-1) Let's spell this out — n_atoms uses an abbreviation that violates our naming conventions. We should use number_of_atoms instead. (See AGENTS.md HARD RULE 4).
| n_atoms: int = Field(default=1, exclude=True) | ||
|
|
||
| @model_validator(mode="after") | ||
| def _derive_grid_metric_value_from_dimensions(self) -> "PointsGridDataProvider": |
There was a problem hiding this comment.
(TB-DOC-2) Let's add a docstring here explaining what this validator does and why it's triggered after initialization. All new methods need docstrings.
| ) -> List[int]: | ||
| raise NotImplementedError | ||
|
|
||
| def calculate_grid_metric(self, grid_metric_type: str, dimensions: List[int], units: str = "angstrom") -> float: |
There was a problem hiding this comment.
(AGENTS.md 1.2. OOP Guidelines & Antipatterns) Why is grid_metric_type typed as a str in the signature but compared to an enum GridMetricType.KPPRA here? Let's ensure the type hint matches the actual usage (e.g. GridMetricType or Union[str, GridMetricType]) to prevent type-checking failures or silent comparison bugs.
|
|
||
| def calculate_grid_metric(self, grid_metric_type: str, dimensions: List[int], units: str = "angstrom") -> float: | ||
| raise NotImplementedError | ||
| if grid_metric_type == GridMetricType.KPPRA: |
There was a problem hiding this comment.
(TB-DRY-1) Let's avoid hardcoding the exact 3 indices here. Using math.prod(dimensions) * self.number_of_atoms (assuming n_atoms gets renamed per the other comment) makes this safer for 1D or 2D grids and a bit cleaner to read.
PointsGridDataProvider had no material, so it defaulted the atom count to 1 and never emitted reciprocalVectorRatios. Both are properties of the material, and the JS provider derives both from the one it is always constructed with. Two consequences, both silent: - KPPRA is per reciprocal atom, so a 2-atom cell on a 4x4x4 grid recorded 64 where the correct value is 128 -- wrong by a factor of the atom count. - The absent reciprocalVectorRatios made the k-grid Important Settings form reject every edit without surfacing an error, so a cloned job's grid could not be changed. The provider now takes `material` and derives both into the ESSE schema fields (not read-time properties, so a schema-driven default_data cannot drop them). When the atom count cannot be derived it raises rather than assuming one atom. `material` is typed structurally via a runtime_checkable Protocol: mat3ra-made ships scipy only under its `tools` extra while importing Material requires it, so a nominal import would make mat3ra.wode unimportable on a plain install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
134 chars against ruff's line-length = 120, which reddened run-py-linter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Inserting `_material_stub` after `import pytest` pushed eight imports below a function definition, which CI's ruff (0.0.270) flags as E402. Verified clean with CI's exact invocation: ruff check --line-length=120 --target-version=py310 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Problem
Clone a job whose k-grid was set explicitly in a notebook, then try to change the k-grid on the clone: nothing happens. The value is stuck, the metric reads
-1, and no error is shown.PointsGridDataProvider(dimensions=SCF_KGRID, isEdited=True)— the exact calltotal_energy.ipynbmakes — never suppliesgridMetricValue. It defaults to theDEFAULT_KPPRAsentinel (-1) and gets persisted as if it were a deliberate, edited value:That is wrong regardless of downstream effect: a sentinel meaning "nothing was ever computed" gets marked
isEdited: Trueand persisted as if it were the user's deliberate choice, and nothing downstream ever recomputes it (checked both sides — Python's_get_effective_dataand JS'sthis.data?.gridMetricValue || this.defaultMetric.value, which treats-1as truthy and passes it straight through).Correction, added after initially opening this PR: I first claimed the observed "locked, can't edit" symptom was caused by the k-grid form's schema requiring
gridMetricValue >= 1for KPPRA, with RJSF re-validating the full form state on every edit and silently rejecting it. That claim was wrong. I verified it against a hand-copied version of the schema, not the actual compiled one; running it against the realKGridFormDataManagerclass shows theminimum: 1constraint never actually reaches the schema RJSF validates against —getPatchedSchemaById's dot-notation merge (applyPatchWithDotNotationin@mat3ra/esse) can't create the missing intermediatedependencieskey that the base ESSE schema doesn't have, so the whole patch silently no-ops. Confirmed:gridMetricValue: -1validatestrueagainst the real schema, same as64. So this PR does not have proof of why editing locks in the UI — only that the persisted-1is wrong on its own terms. Thedependenciespatch being dead code is a separate, likely-unrelated bug worth its own look.Fix
PointsGridDataProvidernow derivesgridMetricValuefromdimensions(× an optionaln_atoms, default 1) whenever dimensions are given explicitly but the metric is not — mirroringPointsGridFormDataProvider.setData()in JS, which already does exactly this when a user sets dimensions manually in the UI. An explicitly passedgridMetricValueis still respected; the untouched default path (no dimensions, no metric at all) is unaffected and correctly keeps the-1sentinel, since there's nothing to derive a value from.Test plan
pytest— 85 passed (baseline 81 + 4 new)[4,4,4], n_atoms=1); respects an explicit override; derives correctly with a realn_atoms; untouched default path keeps the sentinelconvergence_mixin.py's separate call pattern (PointsGridDataProvider(data=kgrid_data)) verified unaffected — it never sets thedimensionsfield, so the new derivation logic doesn't firegit stash)Not fixed here
nAtomsrequires the caller to actually pass a real atom count —n_atomsdefaults to1since this provider has no material context of its own. Left as a follow-up inapi-examples.-1persistence is fixed regardless because it's wrong on its own terms, but I have not located why editing a cloned job's k-grid was observed to silently fail. That likely requires checking@mat3ra/workflow-designer, which isn't checked out in this task.dependenciespatch injsonSchemaPatchConfigbeing silently dropped (dead code — thepreferGridMetric/KPPRA-vs-spacing branching never applies) is a separate bug, filed inlog/findings.md.