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

Atomic level feature draft #2784

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

StLeonidas
Copy link

Remove-me-sectionAtomic level feature draft

  • Recovers atomic level structure from ket file
  • exposed to wasm
  • draft works with MolV3000
  • draft works with MolV3000linear peptides

else {
float tan = dy / dx;
float at = atan(tan);
angle = (dx < 0) ? PI / 2 + at : -PI / 2 + at;

Choose a reason for hiding this comment

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

might be good idea to specify that function finds angle between point and Y axis. also, I believe atan2 is prefered

Copy link
Author

@StLeonidas StLeonidas Feb 25, 2025

Choose a reason for hiding this comment

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

@drizhina Yes, thanks. Possibly will refine all rotation to rays not setions as well

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 you could use float Vec2f::tiltAngle()

@StLeonidas StLeonidas changed the title Atomoc level feature draft Atomic level feature draft Feb 25, 2025
return str;

size_t detectionDomain = str.size() - substr.size() + 1;
detectionDomain = (substr == "\\t" || substr == "\\r" || substr == "\n") ? detectionDomain + 1 : detectionDomain;
Copy link

@skalkin skalkin Feb 25, 2025

Choose a reason for hiding this comment

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

Are you sure about \\n and \\r, but \n (only one \)?

std::string res;
for (size_t i = 0; i < str.size(); ++i) {
if (stop) {
res += str[i];
Copy link

Choose a reason for hiding this comment

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

Can't we do str.replace(...) to avoid multiple allocations?

Copy link
Author

Choose a reason for hiding this comment

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

I will refine to classical str replace as additional feature of replace first is not required any more

@StLeonidas
Copy link
Author

A couple of items before proceeding. @AliaksandrDziarkach, @vanoprenko FYI

  1. what is the most appropriate way to return the molecule? now some ambiguous part with molV3000 is added. molV3000 is possibly the best to return from API when we want to just get the molecule. Though for ketcher part it cold be inconvenient, core deal with its own molecule and molecule base, API deals with its own molecule and molecule base, ket objects seem to also be applied. There is also no option to deal with notations/formats in the core, some parts are in the API, mols as strings are not used in the code now, so this refactoring could be obligatory.
  2. There are several types of builds which are specified as precompilation flags, they are not added now but you might want to see them for different pipelines other then wasm. Can add them, but did not check them currently.
  3. I wonder the reasons KetAtom is inherited class while monomer libraries contain KetAtomBase class, are there any cases when atoms in the library have no sufficient data to be KetAtom?
  4. Tests - the feature itself can face very different cases and it would be great to cover it with multiple tests, would it be fine if I add series of tests for it?
  5. Now step by step transformations are implemented as function to get the resulting molecule to keep the logic clear, though it might be changed at further steps.
  6. As mentioned before I will focus on using internal indigo classes more often, like vectors.
  7. Nearest actions: cover all R1-R2 primitives (broken lines, cycles), add branches (Rn linkages)

@AliaksandrDziarkach
Copy link
Collaborator

I wonder the reasons KetAtom is inherited class while monomer libraries contain KetAtomBase class, are there any cases when atoms in the library have no sufficient data to be KetAtom?
It also can be Atom List or RG label
image

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.

4 participants