-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add validator to check whether anonymous and reduced formulae are reduced #914
Conversation
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
==========================================
+ Coverage 92.77% 92.80% +0.02%
==========================================
Files 67 67
Lines 3765 3780 +15
==========================================
+ Hits 3493 3508 +15
Misses 272 272
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
63f8875
to
64cfd88
Compare
optimade/models/structures.py
Outdated
# Can replace with just `math.gcd(numbers)` in Python 3.9+ | ||
_gcd = reduce(gcd, numbers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make a special import that takes care of this then and you can import so the functionality becomes gcd
?
Does it make sense? Aka. did I make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make sense, but I'm not sure it is any less confusing? If I read _gcd
(or gcd_
) I understand it as avoiding a nameclash in some sense. I think _gcd = reduce(gcd, numbers)
is clearer than from math import gcd as _gcd; gcd = reduce(_gcd, numbers)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you mean a versioned Python import such that it calls math.gcd directly for 3.9 and reduce(gcd, numbers) for <3.9... cba with that really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you mean a versioned Python import such that it calls math.gcd directly for 3.9 and reduce(gcd, numbers) for <3.9... cba with that really.
This ! :) (So I didn't make sense - I just got lucky that you eventually thought of the same thing 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked it slightly to address both my interpretations of your question...
Now for >=3.9 it calls math.gcd(numbers)
directly and the variable is now just gcd
, not gcd_
.
Oh yeah, wanted to add a comment that it might be nice to use some other variable names than |
For some reason I had mentally added |
64cfd88
to
ad1870b
Compare
ad1870b
to
14027c2
Compare
This should be good to go @CasperWA and @JPBergsma. There's now some duplication between validators (i.e. we parse out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me.
I still made a minor point about line 903 in structures.py, but this is mostly me playing code golf, and it is not important.
🎉
I'm a sucker for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two total comments on my side (this plus the old one) - after that this is good to go for me.
14027c2
to
b8cb8f9
Compare
Huh, my pylint doesn't complain about loop variables...
I think we might be talking about outdated changes or something now, unless you want to replace |
b8cb8f9
to
9c19e9c
Compare
…uced - Also explicitly check for 0 chemical proportion in formulae
- Add --version flag for the validator - Do 'minimal' tests first, as this is where actual model deserialization occurs
9c19e9c
to
3195c5b
Compare
Ah - it might have changed then.
Nah, it was just a suggestion for what to name |
Closes #913.