topology featurizers debug

XiaoqZhang commented 3 months ago

Hi, I found a bug in the topology featurizers. The original way of splitting atom_types will split 'Cu' to ['C', 'u'], for example. Besides, the tuple type is error-prone to the last , as highlighted by the red circle in the figure attached.

Thus, I made several changes accordingly:

change the data type of atom_types from tuple to list;
add a function filter_element_for_ph, which is a modified version of filter_element for persistence homology. This function is added instead of modifying the original function because filter_element is also called by mofdscribe.featurizers.chemistry.AMD
uncomment the two lines: https://github.com/kjappelbaum/mofdscribe/blob/44c44f7b9d847105073432b4fb66ad750e6e63b2/src/mofdscribe/featurizers/topology/_tda_helpers.py#L107 and https://github.com/kjappelbaum/mofdscribe/blob/44c44f7b9d847105073432b4fb66ad750e6e63b2/src/mofdscribe/featurizers/topology/_tda_helpers.py#L108

Thermal conductivity

Summary by Sourcery

Fix the atom type splitting issue in topology featurizers by changing the data type from tuple to list and introduce a new function filter_element_for_ph for persistence homology. Uncomment previously commented lines in _tda_helpers.py to restore functionality.

Bug Fixes:

Fix the incorrect splitting of atom types in topology featurizers by changing the data type from tuple to list.

Enhancements:

Introduce a new function filter_element_for_ph to handle element filtering specifically for persistence homology, ensuring compatibility with existing functionality.

sourcery-ai[bot] commented 3 months ago

Reviewer's Guide by Sourcery

This pull request addresses a bug in the topology featurizers of the mofdscribe library. The main changes involve modifying how atom types are handled, introducing a new function for filtering elements in persistence homology calculations, and adjusting data types to improve robustness. The changes span across multiple files in the project, primarily focusing on the topology-related modules.

File-Level Changes

Files	Changes
`src/mofdscribe/featurizers/utils/substructures.py`	Introduced a new function 'filter_element_for_ph' specifically for persistence homology calculations
`src/mofdscribe/featurizers/topology/ph_image.py` `src/mofdscribe/featurizers/topology/ph_hist.py` `src/mofdscribe/featurizers/topology/ph_stats.py` `src/mofdscribe/featurizers/topology/ph_vect.py`	Changed the data type of 'atom_types' from tuple to list in multiple classes
`src/mofdscribe/featurizers/topology/_tda_helpers.py`	Modified the 'get_persistent_images_for_structure' function to use the new 'filter_element_for_ph' function
`src/mofdscribe/featurizers/topology/_tda_helpers.py`	Uncommented two lines in the 'make_supercell' function to include original coordinates and elements
`src/mofdscribe/featurizers/topology/ph_image.py` `src/mofdscribe/featurizers/topology/ph_hist.py` `src/mofdscribe/featurizers/topology/ph_stats.py` `src/mofdscribe/featurizers/topology/ph_vect.py`	Updated the handling of atom types in various classes to accommodate the new list data type

Tips

- Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.

kjappelbaum commented 3 months ago

interesting, thanks for looking into it!

kjappelbaum / mofdscribe