Fix mypy errors on master branch
#3977
Conversation
| if self.solid_compat: | ||
| entries = self.solid_compat.process_entries(entries, clean=True, inplace=inplace, n_workers=n_workers) | ||
| return [entries] | ||
| return [entries] # DEBUG: incompatible return type |
There was a problem hiding this comment.
Incompatible return type, self.solid_compat.process_entries returns list[AnyComputedEntry] and this method should return list[AnyComputedEntry] instead of list[list[AnyComputedEntry]]
There was a problem hiding this comment.
maybe wrapping entries as [entries] was added before the above check?
if isinstance(entries, ComputedEntry):
entries = [entries]do any tests break when you just return entries?
There was a problem hiding this comment.
just noticed that it says "preprocess entries" in the comment above so probably wasn't meant to return at all on this line. tests pass without it. can you confirm @rkingsbury?
There was a problem hiding this comment.
Indeed, there should not have been a return statement on that line. I'm not sure when that was introduced (can't tell from the git blame) but that was actually a pretty serious bug. It was effectively preventing the aqueous compatibility scheme from doing anything when given default arguments.
There was a problem hiding this comment.
@rkingsbury can you think of a good test to add that would have caught this breaking change?
There was a problem hiding this comment.
Indeed, there should not have been a
returnstatement on that line. I'm not sure when that was introduced (can't tell from the git blame) but that was actually a pretty serious bug. It was effectively preventing the aqueous compatibility scheme from doing anything when given default arguments.
I have no recollection of why I would have added this return statement, sadly I think it's likely that I added it to do some sort of manual checking that it worked correctly when run with joblib and then failed to remove it. Sorry.
There was a problem hiding this comment.
for once, mypy proves its worth 😄
in general, can't wait for materials science codes to be fully statically typed!
There was a problem hiding this comment.
@rkingsbury can you think of a good test to add that would have caught this breaking change?
What I would have expected to happen is with default arguments, the ComputedEntry would be returned with the normal solid energy corrections (MaterialsProject2020Compatibility) but no aqueous corrections. I guess existing tests didn't catch it because many (even most) solids won't receive any Aqueous corrections when they are processed. It's only a few materials like diatomic gases and materials containing OH that would get the adjustment.
janosh
added
fix
|
Thanks for reviewing. There was one type error for |
Summary
mypyerrors on master branch, to fix Some type annotations errors when trying to merge master into my local development branch. #3976