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

Element symbols #78

Merged
merged 10 commits into from
May 31, 2021
Merged

Element symbols #78

merged 10 commits into from
May 31, 2021

Conversation

jmaglic
Copy link
Member

@jmaglic jmaglic commented May 30, 2021

No description provided.

Following the discussion in #17, the function strToValidSymbol() inside
the Model class has been adjusted. Valid element symbols are a string of
alphabetic characters including underscore with the first letter in
uppercase and all subsequent letters in lowercase. Any non-alphabetic
characters afterwards are ignored. If the symbol begins with a
non-alphabetic character then the function returns an empty string.

The function is used during structure file and radius file imports and a
error message is displayed, in case of an invalid symbol.

Resolves: #17
@jmaglic jmaglic requested a review from rlavendomme May 30, 2021 16:18
str[i] = tolower(str[i]);
}
// allow underscores inside element symbols for custom symbols
else if (str[i] == '_') {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Underscores don't show properly in unicode chemical formula because it was not considered as a potential character. We should either remove underscore or adapt unicode chemical formula conversion function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm removing underscores for now. If there's a need it's simple to add them back in and adapt the conversion function.

atom_amounts[valid_symbol]++; // adds one to counter for this symbol

// as a safety mechanism, if an element symbol is assigned two atomic numbers, default to 0
// so it becomes apparent laster on that something is wrong
// so it becomes apparent later on that something is wrong
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think elem_Z is not really used at the moment so I don't know what this safety mechanism will change. There is a similar safety mechanism in readFilePDB(). Better to keep elem_Z for future uses though.

Copy link
Collaborator

@rlavendomme rlavendomme left a comment

Choose a reason for hiding this comment

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

Check comments:
Need to make a decision regarding the underscore in element symbols.
Could prevent a crash by checking that radius file contains number in the radius substring.

std::stod() is used in the radius file import to import the radius
values. To make the import more robust, the invalid argument exception
is now handled.
@jmaglic
Copy link
Member Author

jmaglic commented May 31, 2021

Changes have been made. Underscores have been removed as valid symbols, and the app should not be crashing anymore, at least for .xyz and radius files. I'm not equally familiar with .pdb files, so I haven't made all the same changes to that section.

jmaglic and others added 4 commits May 31, 2021 11:22
Skip blank lines in XYZ structure files that were prompting error 105.
Fixed unsignedness comparison warning.
Moved a comment.
@rlavendomme rlavendomme self-requested a review May 31, 2021 13:50
Copy link
Collaborator

@rlavendomme rlavendomme left a comment

Choose a reason for hiding this comment

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

Seems like everything is working as intended.

@jmaglic jmaglic merged commit 760a279 into master May 31, 2021
@jmaglic jmaglic deleted the element-symbols branch May 31, 2021 15:58
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.

2 participants