[WIP] feat: encode and decode using element_coder
#117
Conversation
|
lmk if the pre-commit checks are a pain and I can have it run the normal test suite regardless. |
| fr = frac_coords[i] | ||
| site_ids = np.where(at > 0) |
There was a problem hiding this comment.
As I understand, this was your criterion to identify the number of sites. However, I'm not sure if this will work so well (it will at least require some changes) as some elements will encode to zero and zero values will decode to some valid elements.
Remedies might be to
- separately encode the number of sites
- always ensure that sites occupied with elements are encoded > 0
There was a problem hiding this comment.
Encoding num_sites as a separate variable might be best. While it's more information for a generative algorithm to learn, it might help out with the issue in #82. Originally, I was leaning towards encoding a non-atom as a integer (i.e. 0), but something interesting that happens here is that the distance between a "non-atom" and the lowest atom is the same distance between the lowest atom and the second lowest atom, which seems a bit off to me. If we encode the information separately, then we've essentially encoded parameters for a mask to be applied. That sits a bit better with me.
There's also the possibility of creating a buffer between a "non-atom" and the lowest atom, (e.g. 0 < non_atom < 10).
I think I'll open the encoding num_sites as a separate PR.
| fr = frac_coords[i] | ||
| site_ids = np.where(at > 0) |
There was a problem hiding this comment.
Encoding num_sites as a separate variable might be best. While it's more information for a generative algorithm to learn, it might help out with the issue in #82. Originally, I was leaning towards encoding a non-atom as a integer (i.e. 0), but something interesting that happens here is that the distance between a "non-atom" and the lowest atom is the same distance between the lowest atom and the second lowest atom, which seems a bit off to me. If we encode the information separately, then we've essentially encoded parameters for a mask to be applied. That sits a bit better with me.
There's also the possibility of creating a buffer between a "non-atom" and the lowest atom, (e.g. 0 < non_atom < 10).
I think I'll open the encoding num_sites as a separate PR.
| atomic_symbols = [ | ||
| decode_many( | ||
| encoding, self.element_encoding, metric=self.element_decoding_metric | ||
| ) | ||
| for encoding in unscaled_atom_encodings | ||
| ] |
There was a problem hiding this comment.
This decodes non-atoms to "He" using the mod_pettifor representation (related to atom_range=(0,117) instead of atom_range=(1,118), though it gets dropped later on per your comment.
uniq_atoms = np.unique(list(chain(*atomic_numbers)))
self._atom_range = [np.min(uniq_atoms), np.max(uniq_atoms)]
kjappelbaum
changed the title
element_coder
ToDo:
fitto return a list of elements (as not all scales change the same way across the PTE)encode_manyanddecode_manykjappelbaum/element-coder#7 is implemented (should be easy due to thescirisdata structure we use) switch to theencode_manyanddecode_many