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

Improvements to readCFG.m and writeCFG.m #32

Merged
merged 1 commit into from Nov 24, 2014

Conversation

jat255
Copy link
Contributor

@jat255 jat255 commented Nov 19, 2014

  • Fixed readCFG.m to correctly read CFG files. Now reads whole line (including DW factors, occ., and charge)
  • Fixed writeCFG.m, which was writing the unit vectors in the wrong orientation (relative to readCFG.m). It now writes a file identical to what is read in with readCFG (in my testing)
    • this was causing unit vectors to be transposed, when reading a .cfg in with readCFG.m and then trying to write it with writeCFG.m
    • writeCFG also now explicitly writes the occupancy and everything, which allows it to write a full .cfg file.
      • This may need to be changed in other places throughout the code, as it appears there are a number of places where writeCFG is embedded inside other scripts.
  • Fixed the call to readCFG inside of drawCFG, which caused it not to work.
  • Updated .gitignore to ignore Matlab Editor autosave files

…cluding DW factors, occ., and charge)

Fixed writeCFG.m, which was writing the unit vectors in the wrong orientation (relative to readCFG.m). It now writes a file identical to what is read in with readCFG (in my testing)
writeCFG also now explicitly writes the occupancy and everything, which allows it to write a full .cfg file. This may need to be changed in other places throughout the code, as it appears there are a number of places where writeCFG is embedded inside other scripts.
Fixed the call to readCFG inside of drawCFG, which caused it not to work.
Updated .gitignore to ignore Matlab Editor autosave files
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0a92a7d on jat255:tmp_pull_req into 524dd79 on QSTEM:master.

@msarahan
Copy link
Member

@robbmcleod Can you comment on this, since it's your code being modified? It look good to me, but I don't run Matlab.

@robbmcleod
Copy link
Contributor

I don't see why not. I'll try to test the textscan call to make sure it's working properly, but based on code inspection I see nothing wrong. One issue though is that I don't believe the atomic charge is implemented in a way that's physical? There are some comments by Christoph to this effect.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 99f3433 on jat255:tmp_pull_req into 524dd79 on QSTEM:master.

@jat255
Copy link
Contributor Author

jat255 commented Nov 20, 2014

If it helps, I would say the most critical fixes were:

  • in readCFG.m:
    • The textscan( fHand, '%f%f%f\n', 'CollectOutput', 1 ); command was (in my tests) always only grabbing one line, so it always appeared there was only one atom. As a result, the if (isnan( coordScan(end,2) ) ) condition was never satisfied, so nextAtomZ was never getting assigned a value, and the code was bombing on the atomZ = nextAtomZ line towards the end. Perhaps this is due to differences in Unix vs. Windows line endings? I didn't test on my Linux machine.
    • Since I was fiddling with the textscan command, I figured I'd implement the other parameters (DW, occ, and charge) while I was at it.
  • in writeCFG.m:
    • The main change here was in the line (that now reads) fprintf(fid,'H0(%d,%d) = %10.3f A\n',ix,iy,Mm(iy,ix));. This was Mm(ix,iy) previously, but that was causing the basis vectors to get transposed, which altered the unit cell volume.
    • Like I said, I also changed the code now to write the DW, occupancy, and charge. Even if the atomic charge isn't implemented correctly (yet), it might still be useful to have code around that can keep track of it. It's optional to use at this point and doesn't have to be included when calling writeCFG.
    • writeCFG is included as an embedded function in a lot of other files, and I didn't change those at all, but they may still write files in the wrong orientation.

@jat255
Copy link
Contributor Author

jat255 commented Nov 20, 2014

Sorry, still learning my way through git. I put some changes for a separate pull request on this branch, but now I think I've successfully reverted them, so this branch should only contain the changes mentioned above. The other pull request for the convert2cfg.m script is here.

@msarahan
Copy link
Member

Thanks for the clarification. I'm not concerned about the non-physicality of charge calculations, at least not regarding this commit. In my opinion, that's a separate issue: each part of the code should be as correct and complete as possible. The file reading/writing should maintain the charge information and everything possible, and the rest of the code needs fixing if charge effects are incorrect.

msarahan added a commit that referenced this pull request Nov 24, 2014
Improvements to readCFG.m and writeCFG.m
@msarahan msarahan merged commit 673eaa5 into QSTEM:master Nov 24, 2014
@jat255 jat255 deleted the tmp_pull_req branch November 26, 2014 18:29
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

4 participants