sdfunni / SingleOrigin

Atom-finding Python code
GNU General Public License v3.0
0 stars 0 forks source link

Should the rotated lattice be renamed? #8

Open realitychemist opened 1 month ago

realitychemist commented 1 month ago

When calling rotate_image_and_data the newly rotated lattice has "_rot" appended to its name: https://github.com/sdfunni/SingleOrigin/blob/9cb5de640a71d4be52dfe9cb8a833f04821c28c7/src/SingleOrigin/find_atom_columns.py#L288 This behavior tripped me up: if you use a pattern like img = img.rotate_image_and_lattice("lattice_name", ...) and run this line twice in a row, it will throw a KeyError on the second run.

It is probably better practice anyway to crease a new name for the rotated object (e.g. img_rot) instead of overwriting the old name, and calling it this way avoids raising the KeyError on subsequent runs. However, renaming the lattice isn't mentioned in the function's docstring, so it seems likely that a user could still encounter a KeyError later on in their script if they try to access the rotated lattice with its original name.

I'm suggesting to remove the append, and leave the responsibility of remembering that the lattice has been rotated in the user's hands. If renaming the rotated lattice is definitely the desired behavior, it might be good to do something like:

if "_rot" not in key:
    key += "_rot"

to avoid raising a KeyError on subsequent runs as described above (and it should definitely be documented).

sdfunni commented 3 weeks ago

Added: if '_rot' in key: continue

to skip the current iteration of the loop if the key is already for a rotated lattice. This should overwrite the rotated lattices with new ones. Is that the correct behavior, do you think?