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

All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists #3459

Closed
vanoprenko opened this issue Oct 18, 2023 · 0 comments · Fixed by #3465 or #3486

Comments

@vanoprenko
Copy link
Collaborator

vanoprenko commented Oct 18, 2023

Current State

Currently atom attributes are split onto two categories: General and Query Specific.

General properties represent base chemical attributes (charge, isotope mass, valence, radical) and atom alias. Each of these properties, if defined, has its own way of visualization on canvas:
atom-properties-general

Query Specific properties (ring bond count, H count, substitution count, aromaticity, chirality, etc) represent those properties used in search queries and are displayed as a simple semicolon separated list:
atom-properties-query

There are two problems with current representation of atom attributes:

  1. Some query specific properties may be represented in two different notations: Ketcher (MOL-like) and SMARTS. For example s<n> vs d<n> for substitution count, rb<n> vs x<n> for ring bond count. Other properties have only SMARTS notation, e.g. chirality, aromaticity.
    Properties from the first set are always displayed in Ketcher notation and when mixed in a common list with properties from the second set, resulting set includes a mix of both notations.
  2. When both general and query specific properties are defined for an atom, they may overlap when displayed on canvas, resulting in a broken image:
    atom-properties-clash

Target State (at least one SMARTS)
If at least one SMARTS specific property is defined, the whole set of atom properties (including general and query specific) should be interpreted as SMARTS properties and displayed based on the following rules:

  1. No specific visualization for general properties (charge, valence, etc) should be used. Instead, general properties should be included into the common list of atom properties, together with query specific properties. E.g. instead of showing (IV) for valence, a v4 attribute should be added to the attributes list.
  2. All properties in the list should be displayed in SMARTS notation (see table below).
  3. Similarly, the pop-up window that appears on atom/label hover should display the full list of properties in SMARTS notation.

The list of SMARTS notations for atom properties is as follows:

Property SMARTS notation SMARTS Specific
Label #<n> N
Charge < 0 -<n> N
Charge > 0 +<n> N
Isotope (atomic mass) <n> N
Valence v<n> N
Radical N
Ring bond count x<n> N
Substitution count D<n> N
Unsaturated N
H count H<n> N
Implicit H count h<n> N
Inversion N
Exact change N
Atom list #<n1>,#<n2>,... N
Not atom list !#<n1>,!#<n2>,... N
Special atoms * N
Aromaticity aromatic a Y
Aromaticity aliphatic A Y
Ring membership R<n> Y
Ring size r<n> Y
Connectivity X<n> Y
Chirality clockwise @@ Y
Chirality anticlockwise @ Y

Target State (no SMARTS features)
If no SMARTS features defined, then Ketcher should display the properties the same way as currently (e.g. valence with I,II,III, ring bond count with rb<n>, atom list with [E1,E2,....] except the following changes

  • For atom list display only 8 symbols for the label and show all other symbols on preview
  • Display valence and other non-smarts features for atom-lists and special atoms (similar to default atoms)
@AKZhuk AKZhuk assigned AKZhuk and unassigned Nitvex Oct 18, 2023
@AKZhuk AKZhuk added this to the SMART-Enhancement milestone Oct 18, 2023
AKZhuk added a commit that referenced this issue Oct 19, 2023
@AKZhuk AKZhuk linked a pull request Oct 19, 2023 that will close this issue
9 tasks
AKZhuk added a commit that referenced this issue Oct 19, 2023
… one purely SMARTS attribute exists

- Update e2e snapshots
AKZhuk added a commit that referenced this issue Oct 19, 2023
… one purely SMARTS attribute exists

- Fix isotope position in custom Query
AKZhuk added a commit that referenced this issue Oct 19, 2023
… one purely SMARTS attribute exists

- Fix failed tests
AKZhuk added a commit that referenced this issue Oct 19, 2023
… one purely SMARTS attribute exists

- Fix issue with smarts preview and  template preview
- Fix failed test
AKZhuk added a commit that referenced this issue Oct 20, 2023
… one purely SMARTS attribute exists

- Update logic for aromaticity property
AKZhuk added a commit that referenced this issue Oct 20, 2023
… one purely SMARTS attribute exists

- Update logic for aromaticity property
- Update snapshots
Nitvex pushed a commit that referenced this issue Oct 23, 2023
… one purely SMARTS attribute exists (#3465)

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists
- Update e2e snapshots

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists
- Fix isotope position in custom Query

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists
- Fix failed tests

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists
- Fix issue with smarts preview and  template preview
- Fix failed test

* #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists
- Update logic for aromaticity property
- Update snapshots

* - updated snapshot

---------

Co-authored-by: Mikhail Zhirnov <mikhail_zhirnov@epam.com>
@AKZhuk AKZhuk closed this as completed Oct 23, 2023
AKZhuk added a commit that referenced this issue Oct 23, 2023
…if at least one purely SMARTS attribute exists
AKZhuk added a commit that referenced this issue Oct 23, 2023
…if at least one purely SMARTS attribute exists
Nitvex added a commit that referenced this issue Oct 25, 2023
…if at least one purely SMARTS attribute exists (#3486)

* Backmerge: #3459 - All atom attributes should be displayed as SMARTS if at least one purely SMARTS attribute exists

* - updated snapshot

* Merge branch 'master' into 3459-all-atom-attributes-should-be-displayed-as-smarts-if-at-least-one-purely-smarts-attribute-exists-1

---------

Co-authored-by: Mikhail Zhirnov <mikhail_zhirnov@epam.com>
Co-authored-by: Nikita_Vozisov <Nikita_Vozisov@epam.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment