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

Fix failure to scrape magres files with more than 100 atoms #197

Merged
merged 3 commits into from Aug 24, 2021

Conversation

JordD04
Copy link
Contributor

@JordD04 JordD04 commented Aug 24, 2021

CASTEP magres files store magnetic shielding information in the form:

ms O 99         -2.0523362330721022E+02          3.6848981558575366E+02         -2.2771013606692599E-02          3.5814867568808620E+02         -2.9946873927125921E+02         -1.0428524506521353E+00          7.9458932961222715E-03         -2.2003847124460636E-03         -5.5724953570109824E+02

Where O is the element and 99 is a numerical index. When that index reaches 100, the space after the O is lost, resulting in a line like:

ms O100         -2.0179072731970462E+02          3.6630064905427832E+02          1.2616916920493680E-02          3.5658185566204065E+02         -3.0353628246235576E+02         -1.0440187939142316E+00         -1.5038528456938940E-02          3.7592007872482180E-02         -5.5287819200589945E+02

Applying Python's split function to each line therefore yields:

['ms', 'O', '99', '-2.0523362330721022E+02', '3.6848981558575366E+02', '-2.2771013606692599E-02', '3.5814867568808620E+02', '-2.9946873927125921E+02', '-1.0428524506521353E+00', '7.9458932961222715E-03', '-2.2003847124460636E-03', '-5.5724953570109824E+02']
101
['ms', 'O100', '-2.0179072731970462E+02', '3.6630064905427832E+02', '1.2616916920493680E-02', '3.5658185566204065E+02', '-3.0353628246235576E+02', '-1.0440187939142316E+00', '-1.5038528456938940E-02', '3.7592007872482180E-02', '-5.5287819200589945E+02']

The second list is one element shorter, which resulted in one element of the magnetic shielding being ignored for indices above 99.
magres_scraper now takes the last 9 elements, instead of all elements after the first 3.

JordD04 and others added 2 commits August 24, 2021 14:18
Fixed changes to spacing
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #197 (b9f87f6) into master (1fa94ac) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head b9f87f6 differs from pull request most recent head 66da348. Consider uploading reports for the commit 66da348 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   75.54%   75.56%   +0.01%     
==========================================
  Files          92       92              
  Lines       12371    12371              
==========================================
+ Hits         9346     9348       +2     
+ Misses       3025     3023       -2     
Flag Coverage Δ
unittests 75.56% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
matador/scrapers/magres_scrapers.py 95.19% <100.00%> (ø)
matador/compute/compute.py 44.38% <0.00%> (+0.21%) ⬆️

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 1fa94ac...66da348. Read the comment docs.

Got rid of superfluous tabs.
@ml-evs ml-evs changed the title Magres scraper Fix failure to scrape magres files with more than 100 atoms Aug 24, 2021
Copy link
Owner

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks @JordD04!

@ml-evs ml-evs enabled auto-merge (squash) August 24, 2021 13:50
@ml-evs ml-evs merged commit 4e3d7db into ml-evs:master Aug 24, 2021
@JordD04 JordD04 deleted the magres_scraper branch September 2, 2021 11:27
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.

None yet

2 participants