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

Add the ability to use a toml file as input. #10

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

Conversation

joanimato
Copy link
Contributor

This PR adds the option to use a .toml file as an input file. The .toml file can store command line options for reproducibility. For example, instead of running

scrub.py "c1cc[nH]c(=O)c1" -o scrubbed.sdf --pH 5 --skip_gen3d

you can create a input.toml file with the following options:

input = "c1cc[nH]c(=O)c1"
out_fname = "scrubbed.sdf"
ph = 5
skip_gen3d = true

and then run scrub.py input.toml. This makes it easier to create reproducible runs when testing the software. The toml input also allows the user to input multiple SMILES strings (similar to a .smi file).

CLI options can still be passed to the script and will override anything that's in the toml file.

@joanimato joanimato requested a review from diogomart January 7, 2025 21:55
Copy link
Contributor

@diogomart diogomart left a comment

Choose a reason for hiding this comment

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

Thanks for this feature, I think it's useful. I have two comments:

  1. It looks easy to add bugs if we add options in the future to argparse and forget to update the list of expected keywords in the override function, which is in a different file.
  2. If we have a typo in an option in toml there is no error, it fails silently. It should error.

Similar functionality is implemented in meeko's mk_prepare_ligand.py, by setting the defaults of argparse based on the configuration file. This happens here:
https://github.com/forlilab/Meeko/blob/07a38f8bac4aa9eef4ef30aa8e67475b15d517aa/meeko/cli/mk_prepare_ligand.py#L276

and the defaults that are used for options that are passed neither in the config file or through argparse are read directly from the class:
https://github.com/forlilab/Meeko/blob/07a38f8bac4aa9eef4ef30aa8e67475b15d517aa/meeko/cli/mk_prepare_ligand.py#L50

Do you think that would make sense here?

@joanimato
Copy link
Contributor Author

Sounds good, I will make the modifications.

@joanimato
Copy link
Contributor Author

@diogomart

The new commit should address your issues. I did not want to make any major modifications just yet. We can decide any major regactors (as well as adding a test suite) later.

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.

2 participants