Chore: Architecture migration#295
Merged
Merged
Conversation
- Implement DataAccess[T] and DataContext[T] in data_access.py - __call__ returns iterable of DataContext, supports (raw, ctx) tuple unpacking - from_data classmethod wraps raw data directly for testing/composition - Source resolution: parses selection, matches schema sources, strips token - Add 25 tests covering all paths (passthrough, source resolution, errors) - Update port-quantity SKILL.md to iteration pattern (for raw, _ in self._data():) - Update docs/architecture/calculation.rst to iteration pattern
… results - merge(generator) unwraps single results, combines multiple dicts, returns None for empty/all-None - Add 9 tests covering all merge paths including primary DataAccess usage patterns
…uantity skill update
…ispatch functions
… selection arg, type hints, to_dict)
…gap from_path/from_file/_path
…cher/Handler architecture
…TCAR to Dispatcher/Handler architecture
…al, current_density
All handler to_database() methods now return the DB object directly instead
of {"quantity": DB_object}. Update tests to drop the ["key"] subscript and
update dispatcher tests to expect the DB object at result["default"] rather
than result["default"]["key"].
Introduces merge_to_database in dispatch.py, which wraps _dispatch and remaps result keys from selection names to quantity-prefixed keys: the default selection maps to just the quantity name, non-default selections to quantity_selection. Leading underscores are stripped so private quantities like _CONTCAR appear without the prefix. All dispatcher _to_database methods are updated to use merge_to_database instead of _dispatch, and _collect_to_database in __init__.py is simplified to use the pre-built keys directly. Two placement bugs are also fixed: the Dos and Energy _to_database methods were previously unreachable dead code in the wrong scope.
_result_has_data treated a has_* flag set to False as data, so quantities that are only reachable through a shared structure (local_moment, potential) were emitted with all-None payloads. Treat False has_* flags as empty. Also drop the dunder-field exclusion, which is no longer needed now that __SCHEMA_VERSION__ lives in a global variable instead of the DB dataclasses.
Private quantities are registered with a leading underscore (e.g. _dispersion,
_CONTCAR) so that Calculation.__getattr__ keeps them off the public API. The
schema, however, never uses that underscore, so source.access("_dispersion")
raised FileAccessError, was suppressed, and the quantity silently produced no
database output.
Set _quantity_name to the underscore-stripped schema name in the @quantity
decorator while keeping the registry key (and thus the privacy) intact, and
match the Dispersion constructor default so the standalone from_path path works
too. Dispersion now contributes its eigenvalue range to the database.
StoichiometryHandler.to_database wrapped its result in a {"stoichiometry": ...}
dict, unlike every other handler (which returns the bare DB object and lets
merge_to_database add the key). Return the bare Stoichiometry_DB so the handler
matches the convention and its unit test passes.
Stoichiometry was also never collected by Calculation._compute_database_data
because it is still a base.Refinery (not registered via @quantity), so it never
appeared in the database output. Structure._to_database calls the Refinery's
_read_to_database, so the class cannot drop base.Refinery until Structure is
migrated. Add a thin registered dispatcher that delegates to StoichiometryHandler
for database collection, leaving the Refinery in place for its other consumers.
Replace the legacy Stoichiometry(base.Refinery) class with a single dispatcher class that delegates its public API to StoichiometryHandler, matching every other migrated quantity (Dispersion, LocalMoment, ...). All existing docstrings are preserved on the new dispatcher methods. A _repr_pretty_ is added so that density/current_density __str__ (which call IPython's pretty.pretty) still render. Also: - Remove the dead Structure(base.Refinery)._to_database, the sole caller of Stoichiometry._read_to_database; the live structure database path is StructureHandler.to_database. This decouples Structure from the old Stoichiometry. - Drop the unused calc._stoichiometry property by removing "_stoichiometry" from the legacy QUANTITIES tuple. - Drop now-unused imports (base, raw_data in _stoichiometry; database in structure). Tests: feed test_to_database the raw data via the fixtures instead of the removed Refinery _raw_data attribute, and add dispatcher-level _to_database tests covering both the populated case and the empty (no ion types) case that is filtered out. Structure and Cell migration (and registering them as database quantities) is a follow-up.
…lding Completes the migration of all quantities from the base.Refinery design to the Handler + @quantity dispatcher pattern. Structure & Cell: - Structure is now @quantity("structure") delegating to StructureHandler and is registered as a database quantity. Preserves all docstrings, from_POSCAR/from_ase, slicing, _repr_html_, and a round-trip __repr__. - Deleted Cell(base.Refinery); effective_coulomb, dielectric_tensor, piezoelectric_tensor, and Structure now use CellHandler directly. base.Refinery teardown: - Deleted src/py4vasp/_calculation/base.py (no Refinery subclasses remain). - Moved suppress_and_record/record_encountered_error to _util/error.py. - Removed the dead phonon.Mixin. QUANTITIES rewire onto the registry: - QUANTITIES/GROUPS/GROUP_TYPE_ALIAS/AUTOSUMMARIES are derived from the dispatcher _REGISTRY instead of a hardcoded tuple; removed _make_property/_make_group/_add_all_refinement_classes. calc.<quantity> resolves via Calculation.__getattr__. - The @quantity decorator injects a round-trip __repr__. - database.get_all_possible_keys enumerates keys from the registry; removed the dead AST Refinery-scanning helpers. - _sphinx hidden docstring uses autoclass instead of autoproperty. Tests updated accordingly. `pytest tests` is green except the 6 pre-existing mdtraj-missing test_to_mdtraj failures. Sphinx example tests could not be run locally (sphinx not installed); the example_dos rewrite and doc-gen changes are unverified there.
Database collection is internal and never user-selected, so merge_to_database now drops its selection argument and instead enumerates every unique (non-alias) source of a quantity from the schema, dispatching them in one pass. The default source keys as `quantity`, every other source as `quantity_source`; results that merely duplicate the default (as an in-memory DataSource yields) are collapsed. This restores quantity_source database keys (e.g. structure_final from the top-level structure quantity reading the CONTCAR) that the previous default-only path dropped. FileNotFoundError is suppressed during collection because an optional source may point to an absent external file (the structure poscar source). The selection parameter is removed from every quantity's _to_database.
_write_dataset checked the Link branch before the None check, so a None-valued linked field (e.g. DielectricTensor.cell when method is not "dft_slab") recursed into write() with None and crashed with KeyError: 'none_type'. Test for None data first so such fields are skipped regardless of target type. Also add TODO.md recording out-of-scope issues found while testing: the empty current_density/phonon_band to_database stubs and the ionic()/"ion"-source q_point write mismatch.
_write_dataset wrote numpy unicode arrays (dtype kind "U") directly, which h5py cannot serialize (TypeError: No conversion path for dtype). VASP stores strings as fixed-length byte strings, so encode unicode arrays to bytes before writing, at both the templated and plain write sites. The check gates on .dtype (cheap on VaspData, no materialization) so numeric and non-string data is untouched. This unblocks writing quantities with str labels such as effective_coulomb's spin_labels. Add a round-trip test covering a Stoichiometry with unicode ion_types.
# Conflicts: # src/py4vasp/_calculation/band.py # tests/calculation/test_band.py
_collect_to_database prepended the group name to each result key, but a group member's _to_database already keys by its full _quantity_name (e.g. "phonon_mode" from @quantity("mode", group="phonon")), so the group prefix was added twice, producing "phonon_phonon_mode". Every group member's _quantity_name starts with its group, so the extra prefix is always wrong: merge the results directly and drop the now-unused group_name/quantity_name parameters. Add a dispatch test (standalone key is "phonon_mode") and a calculation-level test (collecting the group member yields "phonon_mode", not the doubled key).
Run isort (black profile) and black over src and tests. Mostly import reordering and reindenting over-indented database-constructor blocks that were introduced during the architecture migration. Two spots where the formatter produced awkward or inconsistent output are adjusted for readability: - dielectric_tensor: rename the local isotropic_dielectric_constant to isotropic_constant so each database argument stays on a single line instead of black exploding the [2] subscript onto its own line. - test_is_suspected_2d_system: extract a small helper so all three asserts read as one-liners rather than a mix of one-liner and method-chain splits.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if ctx.remaining_selection is None and selection_has_default: | ||
| result = method(handler, *args, **kwargs) | ||
| else: | ||
| result = method(handler, ctx.remaining_selection, *args, **kwargs) |
|
|
||
| alpha_2d = (l_vacuum / (4.0 * np.pi)) * (eps_parallel - 1.0) | ||
| return alpha_2d | ||
| return None |
| *_TO_DATABASE_SUPPRESSED_EXCEPTIONS, | ||
| context="compute_elastic_properties.hardness", | ||
| ): | ||
| vickers_hardness = elastic_tensor.get_hardness() |
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Comment on lines
+7
to
+14
| from py4vasp._calculation.dispatch import ( | ||
| DataSource, | ||
| _dispatch, | ||
| merge_default, | ||
| merge_strings, | ||
| merge_to_database, | ||
| quantity, | ||
| ) |
| # Licensed under the Apache License 2.0 (http://www.apache.org/licenses/LICENSE-2.0) | ||
| from __future__ import annotations | ||
|
|
||
| import pathlib |
| from __future__ import annotations | ||
|
|
||
| import pathlib | ||
| from typing import Any, Iterable, List, Optional |
Comment on lines
+14
to
+21
| from py4vasp._calculation.dispatch import ( | ||
| DataSource, | ||
| _dispatch, | ||
| merge_default, | ||
| merge_strings, | ||
| merge_to_database, | ||
| quantity, | ||
| ) |
Comment on lines
+11
to
+20
| from py4vasp._calculation.dispatch import ( | ||
| DataSource, | ||
| _dispatch, | ||
| merge_default, | ||
| merge_graphs, | ||
| merge_strings, | ||
| merge_to_database, | ||
| quantity, | ||
| slice_steps, | ||
| ) |
The abstract base index.Reduction.__init__ requires a keys argument, so the argument-less super().__init__() call raised a TypeError.
martin-schlipf
force-pushed
the
architecture-migration
branch
from
07a72e0 to
9d5c0a2
Compare
Delete TODOs.txt, plan.md, and pytest_output.txt entirely, and untrack TODO.md (kept locally as a working note).
| ) | ||
|
|
||
|
|
||
| class _TensorReduction(index.Reduction): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.