Use get to avoid KeyError on missing cmiles attribute

[ntBre]

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.

• Fixes crash in BasicResultCollection.from_server crashes if an entry is missing a CMILES #299
• Fixes related potential issue for OptimizationResultCollection

Questions

• Should a similar check be performed in TorsionDriveResultCollection? Currently it only tries entry.attributes for the CMILES and will crash if it's not found.

Status

• Ready to go

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.53%. Comparing base (cc3f23e) to head (89b0564).
Report is 8 commits behind head on main.

Additional details and impacted files

[j-wags]

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.

[j-wags]

(blocking) Could you change this to be warnings.warn with a new warning type like MissingCMILESWarning?

[ntBre]

Thanks for the review! I think I covered everything we discussed.

[ntBre]

I also quoted this in one of the commit messages, but I'll copy it here as a reason for switching to logging from warnings. In practical terms I just didn't see an easy way to filter a specific warning class by default as we discussed in the meeting yesterday, but I thought the docs were an interesting additional justification.

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

[j-wags]

Awesome, thanks for the revisions and for looking into logging vs. warnings best practices @ntBre!

Please update the release notes before you merge :-)

[ntBre]

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?