Skip to content

Add FreeSolv#104

Open
jthorton wants to merge 4 commits intomainfrom
freesolv
Open

Add FreeSolv#104
jthorton wants to merge 4 commits intomainfrom
freesolv

Conversation

@jthorton
Copy link
Contributor

Fixes #55
Add FreeSolv ligands with charges and experimental data.

TAG_CHECKS = [ # Each tag should be represented here with necessary files. If no files are necessary, include an empty list.
("protein", ["protein.pdb"]),
("cofactor", ["cofactors.sdf"]),
("sfe", "experimental_solvation_free_energy.json")
Copy link
Member

Choose a reason for hiding this comment

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

Will this work for mnsol given we can't have that file around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that means we just can not tag that dataset :(

- mobley_7176248

- am1bcc_at:
- No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean no ligands had issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh they just hadn't finished yet, in total 1 failed with antechamber and 1 with elf10 so not too bad!

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Overall this seems reasonable to me, couple of questions, and not sure about tests.

@jthorton
Copy link
Contributor Author

Depends on #98 which has modifications to the benchmark data class to capture the experimental data

Copy link
Collaborator

@jaclark5 jaclark5 left a comment

Choose a reason for hiding this comment

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

Looks great! I still need to verify whether we can/can't include MNSol exp. ref data.

The way the exp data file is set up it's the ligand names, so I suppose it's implied that it's an HFE? When I update to include MNSol I can adjust logic to depend on whether a mixture_index.yaml is found containing mixture inch keys and then nothing here will break, does that sound good?

Copy link
Member

Choose a reason for hiding this comment

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

@jaclark5 makes a good point - for consistency we should have the solvent listed in these entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah for MNsol we should have a solute in many solvents so how about we make the keys "solute,solvent" and have identifiers for both in the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file end with "data.json" as described here?

if filename.startswith("experimental") and filename.endswith("data.json"):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!


The reference data was generated using the [generate_freesolv_exp_data.py](../../../data_generation/generate_freesolv_exp_data.py) script using the [conda-lock_linux-64.yml](../../../data_generation/conda-lock_linux-64.yml) environment.
Charges were generated using the [charge_freesolv.py](../../../data_generation/charge_freesolv.py) script using the [conda-lock_linux-64.yml](../../../data_generation/conda-lock_linux-64.yml) environment.
Some ligands could not be charged with all methods, the following lists the ligands that could not be charged with each method:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to add a ligand network for these, or not for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll say no for now, as I don't think this will be needed yet and don't want to confuse people. We can add when needed?

# Conflicts:
#	openfe_benchmarks/data/benchmark_system_indexing.yml
#	openfe_benchmarks/tests/test_benchmark_index.py
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.

Add SFE: Add FreeSolv reference set

4 participants