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

Fixes#1714 => Functionality for setting size of ghost atom #1911

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peach280
Copy link
Contributor

@peach280 peach280 commented Jan 4, 2025

Modified elements.cpp to allow for individual radii for each custom element.
Added methods to set the radii for custom elements.
Updated the methods that return the radii to use the vectors instead of the single values.

Copy link

welcome bot commented Jan 4, 2025

Thanks for opening this pull request! Please check out our contributing guidelines and check for the automated tests.

@ghutchis ghutchis added the enhancement feature changes / API changes label Jan 5, 2025
@ghutchis
Copy link
Member

ghutchis commented Jan 5, 2025

This looks fine from a core perspective. At some point, it needs UI to set the radii.

Before I merge, it would be great to have a test or two in https://github.com/OpenChemistry/avogadrolibs/blob/master/tests/core/elementtest.cpp

@ghutchis ghutchis linked an issue Jan 5, 2025 that may be closed by this pull request
@ghutchis
Copy link
Member

ghutchis commented Jan 5, 2025

Oh, before I forget .. I don't remember if it's mentioned in #1714 but some "ghost" atoms are element 0. That probably needs a special case in your code.

@peach280
Copy link
Contributor Author

peach280 commented Jan 8, 2025

I am working on this but I just wanted to clarify a small doubt, does element 0 mean neutronium? Like the edge case should be for neutronium?

@ghutchis
Copy link
Member

ghutchis commented Jan 8, 2025

Most chemistry packages have a element 0 "Xx" or "Du" - it's a placeholder. In Avogadro, it's used to indicate a centroid point, for example.

@peach280
Copy link
Contributor Author

peach280 commented Jan 8, 2025

I have committed some changes into this including edge case for element 0. Please review.

@avo-bot
Copy link

avo-bot commented Jan 25, 2025

This pull request has been mentioned on Avogadro Discussion. There might be relevant details there:

https://discuss.avogadro.cc/t/running-tests-error/6715/1

@@ -111,9 +113,33 @@
}
}
} CustomElementTableInitializer;
void setCustomElementCovalentRadius(unsigned char atomicNumber, double radius)

Check notice

Code scanning / CodeQL

Unused static function Note

Static function setCustomElementCovalentRadius is unreachable

void setCustomElementVDWRadius(unsigned char atomicNumber, double radius)

Check notice

Code scanning / CodeQL

Unused static function Note

Static function setCustomElementVDWRadius is unreachable
Comment on lines 509 to 572
switch (error_handler)
{
case error_handler_t::strict:
{
JSON_THROW(type_error::create(316, concat("invalid UTF-8 byte at index ", std::to_string(i), ": 0x", hex_bytes(byte | 0)), nullptr));
}

case error_handler_t::ignore:
case error_handler_t::replace:
{
// in case we saw this character the first time, we
// would like to read it again, because the byte
// may be OK for itself, but just not OK for the
// previous sequence
if (undumped_chars > 0)
{
--i;
}

// reset length buffer to the last accepted index;
// thus removing/ignoring the invalid characters
bytes = bytes_after_last_accept;

if (error_handler == error_handler_t::replace)
{
// add a replacement character
if (ensure_ascii)
{
string_buffer[bytes++] = '\\';
string_buffer[bytes++] = 'u';
string_buffer[bytes++] = 'f';
string_buffer[bytes++] = 'f';
string_buffer[bytes++] = 'f';
string_buffer[bytes++] = 'd';
}
else
{
string_buffer[bytes++] = detail::binary_writer<BasicJsonType, char>::to_char_type('\xEF');
string_buffer[bytes++] = detail::binary_writer<BasicJsonType, char>::to_char_type('\xBF');
string_buffer[bytes++] = detail::binary_writer<BasicJsonType, char>::to_char_type('\xBD');
}

// write buffer and reset index; there must be 13 bytes
// left, as this is the maximal number of bytes to be
// written ("\uxxxx\uxxxx\0") for one code point
if (string_buffer.size() - bytes < 13)
{
o->write_characters(string_buffer.data(), bytes);
bytes = 0;
}

bytes_after_last_accept = bytes;
}

undumped_chars = 0;

// continue processing the string
state = UTF8_ACCEPT;
break;
}

default: // LCOV_EXCL_LINE
JSON_ASSERT(false); // NOLINT(cert-dcl03-c,hicpp-static-assert,misc-static-assert) LCOV_EXCL_LINE
}

Check notice

Code scanning / CodeQL

Long switch case Note library

Switch has at least one case that is too long:
replace (52 lines)
.
// previous sequence
if (undumped_chars > 0)
{
--i;

Check notice

Code scanning / CodeQL

For loop variable changed in body Note library

Loop counters should not be modified in the body of the
loop
.
inline std::size_t concat_length(const StringType& str, const Args& ... rest);

template<typename... Args>
inline std::size_t concat_length(const char /*c*/, const Args& ... rest)

Check notice

Code scanning / CodeQL

Unused local variable Note library

Variable rest is not used.
inline std::size_t concat_length(const StringType& str, const Args& ... rest);

template<typename... Args>
inline std::size_t concat_length(const char /*c*/, const Args& ... rest)

Check notice

Code scanning / CodeQL

Unused static variable Note library

Static variable rest is never read.
}

template<typename... Args>
inline std::size_t concat_length(const char* cstr, const Args& ... rest)

Check notice

Code scanning / CodeQL

Unused local variable Note library

Variable rest is not used.
}

template<typename... Args>
inline std::size_t concat_length(const char* cstr, const Args& ... rest)

Check notice

Code scanning / CodeQL

Unused static variable Note library

Static variable rest is never read.
}

template<typename StringType, typename... Args>
inline std::size_t concat_length(const StringType& str, const Args& ... rest)

Check notice

Code scanning / CodeQL

Unused local variable Note library

Variable rest is not used.
@avo-bot
Copy link

avo-bot commented Jan 26, 2025

This pull request has been mentioned on Avogadro Discussion. There might be relevant details there:

https://discuss.avogadro.cc/t/running-tests-error/6715/16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature changes / API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to set size of ghost atom
3 participants