Fix quantity types #106
Fix quantity types #106
Conversation
Pull Request Test Coverage Report for Build 9111179626Details
💛 - Coveralls |
| if not interactions: | ||
| return | ||
|
|
||
| def write_interaction_values(values): |
There was a problem hiding this comment.
In general this function is now a bit confusing to me, I think I do not yet fully understand all your motivations here. Please clarfiy.
There was a problem hiding this comment.
I made this part simpler to get rid of the mapping of the data to the interaction types, now I simply sort the interactions by type and collect the values until a new interaction type is encountered then write the interaction values. I had a temporary fix for the atom_indices and atom_labels because before, the shape (arbitrary) and type (np.object) is not consistent with the definition hence the error. The more formal fix, I think is to restructure the defintion so we get we are not compelled to have a fixed array.
| for key, val in interaction_dict.items(): | ||
| sec_interaction.type = current_type | ||
| sec_interaction.n_atoms = max( | ||
| [len(v) for v in values.get("atom_indices", [[0]])] |
There was a problem hiding this comment.
Should we really give a default of [[0]] if there are no atom_indices instead of just not populating any interactions sections? Can this even occur?
There was a problem hiding this comment.
Yes sure I can put a return here.
atomisticparsers/utils/parsers.py
Outdated
| ) | ||
| for key, val in values.items(): | ||
| # TODO tempory fix: atom_labels, atom_indices not homogeneous | ||
| # fill in missing atom label with 'X', atom index with -1 |
There was a problem hiding this comment.
I don't think it is actually what the function below is doing, and I don't understand what you mean by fill in missing values? I guess you are thinking a case where atom_indices and atom_labels are of a different length than the actual list of atoms? This would be a very strange occurrence from my perspective and should result in an error, unless we have a concrete use case for this.
There was a problem hiding this comment.
From the denition atom_labels and atom_indices have shape ['n_interactions', 'n_atoms'] but n_atoms is not the same for all interactions. Is this right? Hence the fix that I have here that fulfills the definition is to set n_atoms to the max value l189 and for interactions with n-atoms less than max value, I fill in 'X' or -1 respectively. As I have mentioned above, this is not an ideal fix and it is better to redefine Interaction section so we do not group all interactions.
atomisticparsers/utils/parsers.py
Outdated
| if key in ["atom_indices", "atom_labels"]: | ||
| val = [ | ||
| ( | ||
| v |
There was a problem hiding this comment.
This is the reason for the errors in the tests dealing with interactions and the bond list that you have commented out. In the case of "atom_indices" you are subtracting 1 from the atom_index in all cases, which results in a -1-indexed list of indices, and all the test assertions to be off by 1.
atomisticparsers/utils/parsers.py
Outdated
| val = [ | ||
| ( | ||
| v | ||
| + [-1 if key == "atom_indices" else "X"] |
There was a problem hiding this comment.
In line 133/4 of utils/mdanalysis.py I have made a check for atom_names and replaced the labels by X if they do not correspond to actual elements. We should employ a similar logic in general here, but not based on individual missing labels, as mentioned above. If there are no atom label info, then we can replace all by X, otherwise we should check the names/labels individually to see if they correspond to elements. Again, not sure if there exists a case where the atom_label/name list does not match the length of system.atoms. Please clarify if I am wrong.
There was a problem hiding this comment.
i really do not like this fix from me and it is simply wrong. We need to fix the rewrite the definition instead.
There was a problem hiding this comment.
To go back to a flat list, I need to check and adjust my implementation, because this is used for things like the bond list. How do you want to proceed?
There was a problem hiding this comment.
We can perhaps just go for the temporary fix for now, because you are also working on a new schema which probably will not have this complication anyway.
|
@ladinesa I think there are some major misunderstandings happening here. I would like to come to a consensus and fix the issues (i.e., I don't want to wait for the new schema), but would like to chat first. Can you let me know when you are available? |
|
@JFRudzinski can we merge thus now? |
Yes, sorry, I thought that was assumed, please go ahead. |
Fix shape and type inconsistencies due to metainfo refactoring. @TLCFEM