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 bug in parse_as_chemfile, types need to be reversed #19

Merged
merged 2 commits into from
Nov 21, 2023

Conversation

rashidrafeek
Copy link
Contributor

While using chemfiles to parse a structure, I noticed that the atom types where in reverse order in the output vtf file. Due to this the position and species pairing becomes incorrect in the output.

It seems this is due to the types vector being assembled in reverse order (for i in Int(size(frame))-1:-1:0). But all other properties are in normal order. Therefore the types vector need to be reversed. I have verified that this fixes the issue in the output vtf file.

@Liozou
Copy link
Collaborator

Liozou commented Nov 21, 2023

Thank you for the PR! That makes sense yes, the reverse-assembly is necessary so we can do Chemfiles.remove_atom!(frame, i) safely but the types vector should indeed be reversed at the end.

Would it be possible to add a test file? I added this handling of non-cif files long ago but it is lacking tests at the moment. If possible, you could add an example file containing your structures in the test/ folder, and add a line in test/runtests.jl that checks the topology of the file and compares it to what you expect it to find. It would look like

testcase = joinpath(last(_finddirs()), "test", "name_of_your_file") # the path to your file
@test extract1(determine_topology(testcase)) == "the_expected_output"

(if there are multiple intertwinned structures, you may want to remove the extract1 call)

@rashidrafeek
Copy link
Contributor Author

Sure. I have added a test with an xyz file obtained from the already existing "Moganite.cif" file. Also, a test is added for the specific issue of ordering of types, which would fail in the current master. While testing, I noticed that there was an undefined variable in one of the error strings, which I have fixed now.

By the way, thanks for the great package.

@Liozou
Copy link
Collaborator

Liozou commented Nov 21, 2023

Great, thank you for your contribution! That looks good, I'll wait for CI before merging.

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (bcdd0aa) 55.96% compared to head (0e186f9) 55.96%.

Files Patch % Lines
src/input.jl 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
- Coverage   55.96%   55.96%   -0.01%     
==========================================
  Files          16       16              
  Lines        5478     5480       +2     
==========================================
+ Hits         3066     3067       +1     
- Misses       2412     2413       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Liozou Liozou merged commit 6fb53b3 into coudertlab:master Nov 21, 2023
4 checks passed
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.

3 participants