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

#6235 – Incorrect representation of hydrogens for alias charge valence and radical properties in macro mode #6249

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,11 @@ export class MacromoleculesConverter {
reStruct?.bonds.set(bondId, new ReBond(bond));
});

struct.findConnectedComponents();
struct.setImplicitHydrogen();
struct.setStereoLabelsToAtoms();
struct.markFragments();

return { struct, reStruct, conversionErrorMessage };
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BaseRenderer } from 'application/render/renderers/BaseRenderer';
import { Atom } from 'domain/entities/CoreAtom';
import { Atom, AtomRadical } from 'domain/entities/CoreAtom';
import { Coordinates } from 'application/editor/shared/coordinates';
import { CoreEditor, ZoomTool } from 'application/editor';
import { AtomLabel, ElementColor, Elements } from 'domain/constants';
Expand Down Expand Up @@ -163,7 +163,7 @@ export class AtomRenderer extends BaseRenderer {
const isAtomInMiddleOfChain = (atomNeighborsHalfEdges?.length || 0) >= 2;
const hasCharge = this.atom.hasCharge;
const hasRadical = this.atom.hasRadical;
const hasAlias = this.atom.properties.alias;
const hasAlias = this.atom.hasAlias;
const hasExplicitValence = this.atom.hasExplicitValence;
const hasExplicitIsotope = this.atom.hasExplicitIsotope;

Expand Down Expand Up @@ -263,24 +263,27 @@ export class AtomRenderer extends BaseRenderer {
textElement
?.append('tspan')
.attr('dy', this.atom.hasExplicitIsotope ? 4 : 0)
.text(this.atom.properties.alias || this.atom.label);
.text(this.labelText);
}

if (hydrogenAmount > 0) {
if (!this.atom.hasAlias && hydrogenAmount > 0) {
textElement
?.append('tspan')
.attr('dy', this.atom.hasExplicitIsotope ? 4 : 0)
.attr(
'dy',
this.atom.hasExplicitIsotope && shouldHydrogenBeOnLeft ? 4 : 0,
)
.text('H');
}

if (hydrogenAmount > 1) {
textElement?.append('tspan').text(hydrogenAmount).attr('dy', 3);
if (hydrogenAmount > 1) {
textElement?.append('tspan').text(hydrogenAmount).attr('dy', 3);
}
}

if (shouldHydrogenBeOnLeft) {
textElement
?.append('tspan')
.text(this.atom.properties.alias || this.atom.label)
.text(this.labelText)
.attr('dy', hydrogenAmount > 1 ? -3 : 0);
}

Expand Down Expand Up @@ -345,7 +348,7 @@ export class AtomRenderer extends BaseRenderer {
?.append('tspan')
.text(
(Math.abs(charge) > 1 ? Math.abs(charge) : '') +
(charge > 0 ? '+' : '-'),
(charge > 0 ? '+' : ''),
)
.attr('fill', this.labelColor)
.attr('dy', -4);
Expand All @@ -362,7 +365,7 @@ export class AtomRenderer extends BaseRenderer {
this.radicalElement = this.rootElement?.append('g');

switch (radical) {
case 1:
case AtomRadical.Single:
this.radicalElement
?.append('circle')
.attr('cx', 3)
Expand All @@ -376,15 +379,15 @@ export class AtomRenderer extends BaseRenderer {
.attr('r', 2)
.attr('fill', this.labelColor);
break;
case 2:
case AtomRadical.Doublet:
this.radicalElement
?.append('circle')
.attr('cx', 0)
.attr('cy', -10)
.attr('r', 2)
.attr('fill', this.labelColor);
break;
case 3:
case AtomRadical.Triplet:
this.radicalElement
?.append('path')
.attr('d', `M 0 -5 L 2 -10 L 4 -5 M -6 -5 L -4 -10 L -2 -5`)
Expand Down Expand Up @@ -413,6 +416,12 @@ export class AtomRenderer extends BaseRenderer {
const explicitIsotope = this.atom.properties.isotope as number;

this.textElement
/*
* TODO: Currently it's always appended in front of the atom (1H3C or 1CH3), however, in micro mode isotope is placed before the exact atom, not the hydrogen (H31C or 1CH3)
* While the latter is displayed correctly, the former has to be fixed. Can go through all the tspans and use label tspan instead of :first-child here
* However, now it leads to the atom properties being positioned incorrectly due to 'dy' attribute being relative to the previous tspan
* Probably we could consider another approach for positioning the atom properties?
*/
?.insert('tspan', ':first-child')
.text(explicitIsotope)
.attr('fill', this.labelColor)
Expand Down
112 changes: 109 additions & 3 deletions packages/ketcher-core/src/domain/entities/CoreAtom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,18 @@ import { AtomRenderer } from 'application/render/renderers/AtomRenderer';
import { isNumber } from 'lodash';
import { MonomerToAtomBond } from './MonomerToAtomBond';

export enum AtomRadical {
None,
Single,
Doublet,
Triplet,
}

export interface AtomProperties {
charge?: number | null;
explicitValence?: number;
isotope?: number | null;
radical?: number;
radical?: AtomRadical;
alias?: string | null;
}
export class Atom extends DrawingEntity {
Expand Down Expand Up @@ -86,6 +93,10 @@ export class Atom extends DrawingEntity {
return connectionsAmount;
}

public get hasAlias() {
return Boolean(this.properties.alias);
}

public get hasRadical() {
return isNumber(this.properties.radical) && this.properties.radical !== 0;
}
Expand All @@ -105,14 +116,108 @@ export class Atom extends DrawingEntity {
return isNumber(this.properties.isotope) && this.properties.isotope >= 0;
}

private get radicalAmount() {
switch (this.properties.radical) {
case AtomRadical.Single:
case AtomRadical.Triplet:
return 2;
case AtomRadical.Doublet:
return 1;
default:
return 0;
}
}

private get valenceWithoutHydrogen() {
const charge = this.properties.charge || 0;
const label = this.label;
const element = Elements.get(this.label);
// if (!element) {
// // query atom, skip
// this.implicitH = 0;
// return 0;
// }

const elementGroupNumber = element?.group;
const radicalAmount = this.radicalAmount;
const connectionAmount = this.calculateConnections();
const absoluteCharge = Math.abs(charge);

if (elementGroupNumber === 3) {
if (
label === AtomLabel.B ||
label === AtomLabel.Al ||
label === AtomLabel.Ga ||
label === AtomLabel.In
) {
if (charge === -1) {
if (radicalAmount + connectionAmount <= 4) {
return radicalAmount + connectionAmount;
}
}
}
} else if (elementGroupNumber === 5) {
if (label === AtomLabel.N || label === AtomLabel.P) {
if (charge === 1 || charge === 2) {
return radicalAmount + connectionAmount;
}
} else if (
label === AtomLabel.Sb ||
label === AtomLabel.Bi ||
label === AtomLabel.As
) {
if (charge === 1 || charge === 2) {
return radicalAmount + connectionAmount;
}
}
} else if (elementGroupNumber === 6) {
if (label === AtomLabel.O) {
if (charge >= 1) {
return radicalAmount + connectionAmount;
}
} else if (
label === AtomLabel.S ||
label === AtomLabel.Se ||
label === AtomLabel.Po
) {
if (charge === 1) {
return radicalAmount + connectionAmount;
}
}
} else if (elementGroupNumber === 7) {
if (
label === AtomLabel.Cl ||
label === AtomLabel.Br ||
label === AtomLabel.I ||
label === AtomLabel.At
) {
if (charge === 1) {
return radicalAmount + connectionAmount;
}
}
}

return radicalAmount + connectionAmount + absoluteCharge;
}

calculateValence() {
if (this.hasExplicitValence) {
const valence = this.properties.explicitValence as number;
const hydrogenAmount = valence - this.valenceWithoutHydrogen;

return {
valence,
hydrogenAmount,
};
}

const label = this.label;
const element = Elements.get(label);
const elementGroupNumber = element?.group;
const connectionAmount = this.calculateConnections();
const radicalAmount = this.properties.radical || 0;
const absCharge = 0;
const radicalAmount = this.radicalAmount;
const charge = this.properties.charge || 0;
const absCharge = Math.abs(charge);
let valence = connectionAmount;
let hydrogenAmount = 0;

Expand Down Expand Up @@ -358,6 +463,7 @@ export class Atom extends DrawingEntity {
if (connectionAmount + radicalAmount + absCharge === 0) valence = 1;
else hydrogenAmount = -1;
}

// if (Atom.isHeteroAtom(label) && this.implicitHCount !== null) {
// hydrogenAmount = this.implicitHCount;
// }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1981,6 +1981,7 @@ export class DrawingEntitiesManager {
monomerToNewMonomer.get(atom.monomer) as BaseMonomer,
atom.atomIdInMicroMode,
atom.label,
atom.properties,
);
const addedAtom = atomAddCommand.operations[0].atom as Atom;

Expand Down
Loading