Skip to content
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

Some validation QoL tweaks #948

Merged
merged 3 commits into from Sep 28, 2021
Merged

Some validation QoL tweaks #948

merged 3 commits into from Sep 28, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 22, 2021

This PR addresses a few ergonomics/QoL concerns with the validator:

  • Use single precision FP mach eps for elements_ratios validator (closes 'elements_ratios' model validator uses double-precision machine epsilon - could be relaxed #947)
  • Use set operations in some of the chemical symbol field validators (might revert as it doesn't do quite what I wanted to, doubt there's any real speed boost) Dropped this for now.
  • Tweak how 501 errors are handled in validator error messages - try to only give one error that uses the details from the implementation itself
  • Simplify invalid chemical symbol error message by not printing the list of all elements.

@ml-evs ml-evs marked this pull request as draft September 22, 2021 21:34
@ml-evs ml-evs force-pushed the ml-evs/validator_updates branch 2 times, most recently from de90193 to fdf81af Compare September 23, 2021 08:19
@ml-evs ml-evs marked this pull request as ready for review September 23, 2021 08:19
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #948 (5587980) into master (704fdd2) will increase coverage by 0.13%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   92.78%   92.92%   +0.13%     
==========================================
  Files          67       67              
  Lines        3786     3787       +1     
==========================================
+ Hits         3513     3519       +6     
+ Misses        273      268       -5     
Flag Coverage Δ
project 92.92% <90.90%> (+0.13%) ⬆️
validator 92.92% <90.90%> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
optimade/validator/validator.py 83.05% <85.71%> (+0.98%) ⬆️
optimade/models/structures.py 95.84% <100.00%> (-0.02%) ⬇️

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 704fdd2...5587980. Read the comment docs.

optimade/models/structures.py Outdated Show resolved Hide resolved
optimade/models/structures.py Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the remark about the description of EPS the changes seem good to me.

@ml-evs ml-evs merged commit d2bb810 into master Sep 28, 2021
@ml-evs ml-evs deleted the ml-evs/validator_updates branch September 28, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'elements_ratios' model validator uses double-precision machine epsilon - could be relaxed
3 participants