Use get to avoid KeyError on missing cmiles attribute
#300
Conversation
There was a problem hiding this comment.
Thanks @ntBre. Could you change the print to a warning and try add a test here? Maybe load the real dataset from the original issue and ensure it doesn't crash any more (it's fine if this adds up to 10 sec to CI). Also add a pytest.warns to the test to ensure the warning gets emitted.
It'd be cool to have a helpful message for users who encounter this exact issue in the future, but (per our discussion in our one-on-one) we don't have a preferred workaround for this issue yet, so it's not necessary for this PR to add that.
openff/qcsubmit/results/results.py
Outdated
| "canonical_isomeric_explicit_hydrogen_mapped_smiles" | ||
| ] | ||
| ) | ||
| if not cmiles: | ||
| print(f"MISSING CMILES! entry = {entry_name}") |
There was a problem hiding this comment.
(blocking) Could you change this to be warnings.warn with a new warning type like MissingCMILESWarning?
|
Thanks for the review! I think I covered everything we discussed. |
based on these docs (https://docs.python.org/3/howto/logging.html#when-to-use-logging) warnings should be used "if the issue is avoidable and the client application should be modified to eliminate the warning," (like a deprecation warning) while logging a warning should be used "if there is nothing the client application can do about the situation, but the event should still be noted." I think this warning is closer to the latter case, where there's not much a user can do about this, and this allows us to filter these warnings by default, which I don't think is possible with the warnings module
for more information, see https://pre-commit.ci
|
I also quoted this in one of the commit messages, but I'll copy it here as a reason for switching to
|
|
Thanks again for the review! I've updated the release notes too. While I was there, I noticed that a couple of the recent entries have minor typos: The first is missing a link and the second has an extra colon (like I had initially too!). Can I fix those while I'm here or should that be separate? |
Description
This fixes the crash described in #299, although it doesn't add any additional checks to warn if every entry was skipped.
Todos
Notable points that this PR has either accomplished or will accomplish.
OptimizationResultCollectionQuestions
TorsionDriveResultCollection? Currently it only triesentry.attributesfor the CMILES and will crash if it's not found.Status