WIP: Added instance method to generate MolBar from Structure

[crjacinto]

Closes Issues

Closes #227

Description

• Added an instance method that generates the Molecular Barcode from Structure.

Release Notes

(Project adheres to Keep a Changelog; Every entry should start in upper-case and end with (#<pr-id>); Remove sections that don't apply)

Changed

• Modified structure.py.

[haneug]

@crjacinto please repair the title and the description of the PR

[crjacinto]

@crjacinto please repair the title and the description of the PR

Done

[haneug]

Nice PR! Please split the function according to the return types and change the docstring to numpy style. Also an enum for the mode would be a good thing for verification I think.

[haneug]

Please set up a string enum to verify that valid string options are passed. Something like this:

class MolBarMode(StrEnum):
    MB = "mb"
    TOPO = "topo"

and then you can do:

  try:
      mode = MolBarMode(mode)
  except ValueError:
      raise ValueError(f"Invalid mode {mode!r}. Must be one of {[m.value for m in MolBarMode]}")

Either use StrEnum or use StringEnum from opi/src/opi/models/string_enum.py if it can be case insensitive.

[haneug]

Please split this into two separate functions, one returning a str, one returning tuple[str, dict[str, Any]. A function that changes its return type based on a keyword argument makes type hints unreliable and is easy to misuse. Maybe make to_molbar return the string and to_molbar_dict or to_molbar_full return the whole data.
Also please document or give the link to the documentation what this whole data even means.

[haneug]

Could this also be list[tuple[float,float,float]] since we only have x, y, z?

[haneug]

You could check for if self.atoms before trying to loop over them and then the error message would fit better.

[haneug]

This step seems redundant to me. You already converted everything to floats?

[haneug]

I would directly go with the default and I do not directly see the advantage of setting this via a keyword argument. One can always just change self.charge?

[haneug]

This could be problematic when you have fragments in your structure. Please check that as well and use atom.element instead!

[haneug]

Also you should make sure to just include actual atoms and not e.g., point charges do that by checking if it is an instance of Atom

[haneug]

and use atom.coordinates for x,y,z

[timmyte]

How about adding a Structure.get_coordinates() which returns all coordinates as single numpy array.
This can easily be converted to a list of lists using ndarray.tolist().

[timmyte]

Good PR. Thanks for adding this MolBar.

Import statements should always be on module level. There are more less only two reasons to do them on a lower level:

1. For performance, e.g. to speed up CLI auto-completion.
2. If it would lead to a circular import.

It does not seem to me, that either criteria is met here.

The import could look as follows:

from functools import wraps

try:
  from molbar.barcode import get_molbar_from_coordinates
except ImportError:
  # > Making sure the identifier is the global namespace
  get_molbar_from_coordinates = None

def requires_molbar(func):
    @wraps(func)
    def wrapper(*args, **kwargs):
      if get_molbar_from_coordinates is None:
        raise raise ImportError("MolBar is not installed. Install it with: pip install molbar")
      return func(*args, **kwargs)
    return wrapper

# [...]

@requires_molbar
def to_molbar(...):

[timmyte]

How about adding a Structure.get_coordinates() which returns all coordinates as single numpy array.
This can easily be converted to a list of lists using ndarray.tolist().