Add validator to check whether anonymous and reduced formulae are reduced

[ml-evs]

Closes #913.

[codecov]

Codecov Report

Merging #914 (63f8875) into master (7d25322) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 63f8875 differs from pull request most recent head 3195c5b. Consider uploading reports for the commit 3195c5b to get more accurate results

@@            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              

Flag	Coverage Δ
project	92.80% <100.00%> (+0.02%)	⬆️
validator	92.80% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/models/structures.py	96.19% <100.00%> (+0.21%)	⬆️
optimade/validator/__init__.py	100.00% <100.00%> (ø)
optimade/validator/validator.py	82.07% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d25322...3195c5b. Read the comment docs.

[CasperWA]

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?

[ml-evs]

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).

[ml-evs]

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.

[CasperWA]

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 😅)

[ml-evs]

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_.

[CasperWA]

Oh yeah, wanted to add a comment that it might be nice to use some other variable names than v and n (although, I think the first one is used throughout, but I think value should work as well at least - then we can change it more throughout the code later).

[ml-evs]

Oh yeah, wanted to add a comment that it might be nice to use some other variable names than v and n (although, I think the first one is used throughout, but I think value should work as well at least - then we can change it more throughout the code later).

For some reason I had mentally added v to the list of variable names enforced by pydantic validators (alongside field, values etc.), but this is not the case. I can change it to value here. Do you really dislike n that much as a loop variable?

[ml-evs]

This should be good to go @CasperWA and @JPBergsma.

There's now some duplication between validators (i.e. we parse out numbers and elements) for various fields, but there are some subtleties that make this way clearer than doing it all in one go.

[JPBergsma]

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.

[CasperWA]

For some reason I had mentally added v to the list of variable names enforced by pydantic validators (alongside field, values etc.), but this is not the case. I can change it to value here.

🎉

Do you really dislike n that much as a loop variable?

I'm a sucker for pylint's variable naming - what can I say?
It could be reduced_number or something descriptive? It's nice with descriptive variable names - but it's not a deal breaker; just a pet peeve.

[CasperWA]

Two total comments on my side (this plus the old one) - after that this is good to go for me.

[ml-evs]

I'm a sucker for pylint's variable naming - what can I say?

Huh, my pylint doesn't complain about loop variables...

It could be reduced_number or something descriptive? It's nice with descriptive variable names - but it's not a deal breaker; just a pet peeve.

I think we might be talking about outdated changes or something now, unless you want to replace _gcd with reduced_number?

[CasperWA]

I'm a sucker for pylint's variable naming - what can I say?

Huh, my pylint doesn't complain about loop variables...

Ah - it might have changed then.

I think we might be talking about outdated changes or something now, unless you want to replace _gcd with reduced_number?

Nah, it was just a suggestion for what to name n - but no matter.