dependance on chemaxon is annoying

[UnixJunkie]

maybe switch to Dimorphite-DL:
https://jcheminf.biomedcentral.com/articles/10.1186/s13321-019-0336-9

I am in academia and I don't even have a chemaxon license anymore...
Software vendors always change their license terms one day or another...

[DrrDom]

Agree, chemaxon change of an academic license is inappropriate. I now consider different options. Previously I tested Dimophite-DL, pkasolver and chemaxon and at the first glance chemaxon performed better in complex cases. However, I would prefer a more systematic study. At least it would be reasonable to compare Dimorphite-DL and pkasolver. Unfortunately, this will take time, so it will not be fast, but we will try different alternatives. Actually, this could be an opportunity to develop something new

[DrrDom]

It is still possible to disable chemaxon protonation (--no_protonation) and use some external tools to protonate structures before docking

[UnixJunkie]

as another hackish alternative, there would be obabel's -p option (Add Hydrogens appropriate for pH)

[DrrDom]

I implemented an easy way to integrate protonation tools. I also implemented dimorphite as an example in the separate branch dimorphite-example.

What I dislike with dimorphite

• it is not a package and integration of third-party programs as an integral part make it hard to update and support. Dimorphite is pretty old and currently is not developed, but avoiding inclusion of third-party software is a general rule.
• I did not test, but dimorphite may be slow on large sets of compounds, because it works in a single process.
• license of dimorphite is Apache 2 and differs from easydock BSD-3. They are compatible, but if it will become an integral part, easydock should have two licenses.
• it is impossible to use dimorphite API (function run), because it catches argparse of main program arguments. This requires fixes of dimorphite code.

I would prefer if dimorphite will be converted into a package to be installed by pip and can be run using pool on multiple cores. Ideally, if a bug with catching the main argparser using dimorphite API will be also fixed.

If someone will help with that, it will be faster to integrate dimorphote.

@Feriolet, would you be able to help with that?

[Feriolet]

Yes, these have been some of the pain points of using Dimorphite-DL for me as well. I have tried to run 250k compounds, but the init_db ran slowly (around 7 hours). Dimirphite-DL may be the cause, but I have not tested it.

It is also annoying the argparse of Dimorphite is problematic so I cant be used it as a package, and the current solution I have uses subprocess.

I can try to fix the bug of Dimorphite_DL

[Feriolet]

I have found the reason for the argparse error for dimorphite_dl. The main reason that the dimorphite catches the main program argument is because of the command line args = vars(parser.parse_args()) where .parse_args() collect the argument from the terminal as the default (main run_dock argument). To override the default setting and use the desired argument, we have to put the list ['--argument',value,'--argument',value] inside the .parse_args(list).

Since the **kwargs returns a dict_output, I have made a temporary function that converts the dict into a flat list:

def convert_kwargs_to_sys_argv(params: dict) -> list:
    sys_argv_list = []
    for key, value in params.items():
        # Since some argument have store_true argument, we don't need to include the 'True' value into the sys.argv
        if value is True:
            sys_argv_list.append('--'+key)
        elif value is False:
            continue
        else:
            sys_argv_list.append('--'+key)
            sys_argv_list.append(str(value))
        
    return sys_argv_list

Then, the main() function of the dimorphite_dl is modified to:

def main(params=None):
    """The main definition run when you call the script from the commandline.

    :param params: The parameters to use. Entirely optional. If absent,
                   defaults to None, in which case argments will be taken from
                   those given at the command line.
    :param params: dict, optional
    :return: Returns a list of the SMILES strings return_as_list parameter is
             True. Otherwise, returns None.
    """
    sys_argv_list = convert_kwargs_to_sys_argv(params)
    parser = ArgParseFuncs.get_args()
    args = vars(parser.parse_args(sys_argv_list))
    print(args)
    if not args["silent"]:
        print_header()

    # Add in any parameters in params.
    if params is not None:
        for k, v in params.items():
            args[k] = v

    # If being run from the command line, print out all parameters.
    if __name__ == "__main__":
        if not args["silent"]:
            print("\nPARAMETERS:\n")
            for k in sorted(args.keys()):
                print(k.rjust(13) + ": " + str(args[k]))
            print("")

    if args["test"]:
        # Run tests.
        TestFuncs.test()
    else:
        # Run protonation
        if "output_file" in args and args["output_file"] is not None:
            # An output file was specified, so write to that.
            with open(args["output_file"], "w") as file:
                for protonated_smi in Protonate(args):
                    file.write(protonated_smi + "\n")
        elif "return_as_list" in args and args["return_as_list"] == True:
            return list(Protonate(args))
        else:
            # No output file specified. Just print it to the screen.
            for protonated_smi in Protonate(args):
                print(protonated_smi)

[DrrDom]

Good first step. Thank you!

I checked the code of Dimorphite for parallelization and may say it can be not the most difficult part. The code of a generator class looks quite complex and I do not understand whether this complexity is necessary or not.

[Feriolet]

Yes, it will take time for me to understand the code as it uses multiple Classes in huge chunk as well.

[Feriolet]

I noticed that the Protonate() class is an iterator with the __next()__ and __iter()__. Is it even possible to use multiprocessing for this type of class? Calling the Protonate(args) may immediately loop the smi list before we can multiprocess it.

EDIT: nvm it seems that a for loop calles for the next() function of an iterator. I'll see if I can call multiprocessing within/outside of the class type.

[DrrDom]

In theory it should be possible, but I do not have experience with such classes. I usually implement such kind of things in a different way

A very simple example:

with Pool(ncpu) as pool:
    with open(smi_filename) as f:
        for mol_id, protonated_smi in pool.imap_unordered(Protonate, f):
             if protonated_smi:
                  update_db(mol_id, protonated_smi)

where Protonate would take a line and returns mol_id and protonated_smi. f is actually a generator that avoids memory overflow.

There are some examples of generators working with multiprocessing - https://codereview.stackexchange.com/questions/259029/compute-the-outputs-of-a-generator-in-parallel, but those implementations are also simpler.

[Feriolet]

I have tried to understand how iterator class work and I think I got a few ideas on what to do. The dimorphite class needs to take the input filename, and the iterator is generated by the LoadSMIFile() class and stored in the args["smiles_and_data] when the ArgParseFuncs.clean_args() is called. Because of the nature of Iterator class which has the next(iterator) function, the input must be an iterator type, not iterable type.

The above code probably won't work because the Protonate() will treat f as the args input, which is not what we wanted.

My idea is that we can either change how the Protonate() treats the input by separating the args["smiles_and_data"] code from the args which means we have to add another argument Protonate.__init__(self, args, smi_iterator). But I don't know if it will break how dimorphite intends to run the code. Another ridiculous way is that we can split the smi files to n number of smi (e.g., 250k smi will have 250k separate files), which can be easily multiprocess and we don't need to modify the code too much. But again, generating 250k files is kind of ridiculous.

If we plan to change the code, we can probably make the Protonate() take the input of iter([iter(smi),iter(smi),...] where smi represents one smi string with the following function:

def convert_list_to_iter_in_iter(input_list: list):

    iter_list = [iter(str(element)) for element in input_list]
    return iter(iter_list)

then, we can use pool.imap_unordered(Protonate, iter(iter_list)) so the input as treated as iterator rather than a list/string

[DrrDom]

The idea of multiple input files is very nice, because we nevertheless will store all these data on a disk and it does not matter in how many files.

We can create 2x-3x files as the number of CPUs submitted by a user in the command line (argument -c), propagate this ncpu argument to the add_protonation function and use it to run multiples input files using pool and subprocess. Not a perfect solution but fast and working.

If the speed will be too slow, we may think to use dask for this task, but I hope it will be reasonably good.

[DrrDom]

You may create a package of dimorphite-dl under your account. Uploading it to the pypi is not necessary, we may specify in README to install dimorphite directly from your repository.

[Feriolet]

By installing, do you mean through git clone in the repo?

Btw, using 1 cpu to protonate 250k requires 1037s. So, I guess that using the ncpu would be sufficient if we want to scale up the protonation