Sparse set_atom_data with by_species and by_index (closes #65)#72
Merged
Sparse set_atom_data with by_species and by_index (closes #65)#72
Conversation
_coerce_sparse_atom_data now infers dtype from the supplied values: numeric values produce a float array with NaN fill; string values produce an object-dtype array with None fill; mixed types raise TypeError.
Passing a dict to values= now raises TypeError with a pointer to by_index=. All existing dict-in-values call sites in the tests have been migrated to by_index=. The test_sparse_dict_empty_raises test is deleted (covered by the pre-existing test_all_omitted_raises).
There was a problem hiding this comment.
Pull request overview
This PR extends StructureScene.set_atom_data to support sparse per-atom metadata assignment via by_species and by_index, updates the documentation/examples accordingly, and adjusts tests to reflect the new API (including disallowing dicts as positional values).
Changes:
- Add
_coerce_sparse_atom_data()and newset_atom_data(..., by_species=..., by_index=...)sparse assignment forms, withby_indexprecedence and 1-D→2-D promotion rules. - Remove support for passing a dict as positional
values(now aTypeError), and update examples/usages to useby_index=. - Update docs/tests for categorical sparse fill semantics (
Nonerather than empty string).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/hofmann/model/structure_scene.py |
Implements sparse coercion and new set_atom_data API/validation paths. |
tests/test_model/test_structure_scene.py |
Adds comprehensive coverage for by_species/by_index, promotion rules, precedence, and new error behavior. |
tests/test_rendering/test_painter.py |
Migrates rendering tests to the new by_index= sparse form. |
src/hofmann/model/colour.py |
Updates documentation example code to use by_index=. |
docs/colouring.rst |
Documents sparse assignment and updates missing categorical sentinel to None. |
docs/changelog.rst |
Adds user-facing changelog entries for the new API and behavior changes. |
docs/_static/generate_images.py |
Updates image-generation script calls to use by_index=. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR extends StructureScene.set_atom_data() to support sparse per-atom metadata assignment via by_species and by_index, updates documentation/examples accordingly, and adjusts tests to reflect the new API (including the removal of dict-as-positional-values).
Changes:
- Add sparse metadata coercion (
by_species/by_index) with precedence and 1-D/2-D promotion rules. - Deprecate/remove dict positional
valuesinput in favor ofby_index=. - Update tests and docs to use the new sparse assignment forms and to document
Noneas the categorical “missing” fill value.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_rendering/test_painter.py | Updates rendering tests to use by_index= instead of dict positional values. |
| tests/test_model/test_structure_scene.py | Replaces old sparse-dict tests with comprehensive by_species/by_index coverage and new error expectations. |
| src/hofmann/model/structure_scene.py | Implements _coerce_sparse_atom_data() and extends set_atom_data() signature/validation/docs. |
| src/hofmann/model/colour.py | Updates docstring examples to use by_index= sparse form. |
| docs/colouring.rst | Documents sparse assignment patterns and updates missing categorical sentinel from "" to None. |
| docs/changelog.rst | Adds changelog entries for the new sparse API and behavioral changes. |
| docs/_static/generate_images.py | Updates image-generation script to use by_index= for sparse calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Summary
set_atom_datagainsby_speciesandby_indexkeyword arguments for sparse per-atom metadata assignment.by_speciesmaps species labels to scalar, 1-D, or 2-D values;by_indexmaps atom indices to scalar or 1-D values. Both can be combined in one call, withby_indextaking precedence at overlapping atoms.set_atom_datano longer accepts a dict as its positionalvaluesargument -- useby_index=instead.None(object-dtype) instead of empty strings.by_speciesvalues or 1-Dby_indexvalues promote the output to(n_frames, n_atoms). Scalar and 1-Dby_speciesvalues broadcast across frames automatically.Closes #65.