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

Zero occupancy in cif file treated as 1.0. #2125

Closed
orex opened this issue Feb 7, 2020 · 4 comments
Closed

Zero occupancy in cif file treated as 1.0. #2125

orex opened this issue Feb 7, 2020 · 4 comments

Comments

@orex
Copy link
Contributor

orex commented Feb 7, 2020

Dear OpenBabel developers.
Many thanks for your program, and sorry for avoiding the template.

The problem appears in commit bf22042 and now in master.
The issue is that the CIF file occupancy values can be treated incorrectly.

commit bf2204231bfc2bf1a97bf193fa56c3d72ff0c97b
Author: Alexandr Fonari <alexandr.fonari@gmail.com>
Date:   Mon Mar 19 11:39:33 2018 -0400

    Update mmcifformat.cpp

diff --git a/src/formats/mmcifformat.cpp b/src/formats/mmcifformat.cpp
--- a/src/formats/mmcifformat.cpp
+++ b/src/formats/mmcifformat.cpp
@@ -777,1 +777,3 @@
-               occup->SetValue(token.as_number());
+               double occup = token.as_number();
+               if (occup <= 0.0 || occup > 1.0){
+                  occup = 1.0;


The occupancy become 1.0 if it is 0.0. This affect supercell program. orex/supercell#28
I would like to get a comment from @afonari about the reason of such behaviour. If there are no "strong" reasons for this, I propose to change the code to

occup = std::max(0.0, std::min(1.0, occup));

Sincerely yours,
Kirill.

@afonari
Copy link
Contributor

afonari commented Feb 10, 2020

There is no strong reason for this. I think your change is good.

@afonari
Copy link
Contributor

afonari commented Feb 25, 2020

@orex are you going to create a pull request?

@orex
Copy link
Contributor Author

orex commented Feb 25, 2020

Hello. I would like to create a pull request, but I'm extremely busy this week. I don't' want to create it w/o testing.

@afonari
Copy link
Contributor

afonari commented Feb 25, 2020

Thanks, sounds good, no rush.

@orex orex mentioned this issue Mar 6, 2020
ghutchis added a commit that referenced this issue Apr 3, 2020
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

No branches or pull requests

2 participants