Skip to content

Tyk2 alchemicalarchive example - #127

Open
jaclark5 wants to merge 7 commits into
mainfrom
tyk2_alchemarch
Open

Tyk2 alchemicalarchive example#127
jaclark5 wants to merge 7 commits into
mainfrom
tyk2_alchemarch

Conversation

@jaclark5

Copy link
Copy Markdown
Collaborator

For jacs_set/tyk2 with 1 repeat:

  1. Use openfe-benchmarks/scripts/_tmp_alchemiscale_gather.py to pull AlchemicalArchive from alchemiscale
  2. Use openfe-benchmarks/scripts/example_generate_results.py to generate computational_results.json from the alchemical archive
  3. Use openfe-benchmarks/scripts/prepare_metadata_submission.py to generate the zenodo description and submission.yaml

Questions:

  1. How can I get the env package versions from items run on alchemiscale?
  2. Is it ok that if the number of repeats is 1 that I set the uncertainty to NaN?

@jaclark5
jaclark5 requested a review from jthorton June 22, 2026 22:29
tmp_path = tmp.name

try:
archive = AlchemicalArchive.from_json(tmp_path)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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


if "unit_estimate" in outputs:
estimate_qty = outputs["unit_estimate"]
estimate = estimate_qty.magnitude

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use consistent units for the analysis and do m_as


# Extract estimate from the DAG result
if dag_results_list:
dag_result = dag_results_list[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to use the repeat results if they are present


@click.command()
@click.option(
"--archive",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do we want to handle results with many archives, the archive generation script can produce multiple in a single run https://github.com/OpenFreeEnergy/openfe-benchmarks/blob/main/openfe_benchmarks/scripts/_tmp_alchemiscale_gather.py. Is the user expected to then combine the computational_results.json files after or should we adapt this script to accept multiple inputs.

complex_dg_uncertainty = np.nan * unit.kilocalories_per_mole
else:
complex_dg_uncertainty = (
np.std(complex_data) * unit.kilocalories_per_mole

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
np.std(complex_data) * unit.kilocalories_per_mole
np.std(complex_data, ddof=1) * unit.kilocalories_per_mole

solvent_dg_uncertainty = np.nan * unit.kilocalories_per_mole
else:
solvent_dg_uncertainty = (
np.std(solvent_data) * unit.kilocalories_per_mole

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
np.std(solvent_data) * unit.kilocalories_per_mole
np.std(solvent_data, ddof=1) * unit.kilocalories_per_mole

if complex_data and solvent_data:
complex_dg = np.mean(complex_data) * unit.kilocalories_per_mole
if len(complex_data) == 1:
complex_dg_uncertainty = np.nan * unit.kilocalories_per_mole

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will stop cinnabar from running, if we have no repeat uncertainty, we should report the mbar error, we should probably report this for each repeat as well this was missed from the first submission :D

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This won't stop cinnabar anymore, because it will use the mbar uncertainty instead.

gathered_results["dg"].append(entry_data)
except Exception as e:
logger.warning(
f"Could not generate absolute values (DG) for the alchemical map: {e}. "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can still report DGs using the mbar error as the edge uncertainty, but we need to make this clear in the results.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jthorton in the dogs it lists as NaN. Maybe we add something to the dg result with a field like uncertainty_used='mbar'/'replicate'?

@jthorton

Copy link
Copy Markdown
Contributor
  1. How can I get the env package versions from items run on alchemiscale?

I am not sure about this one @IAlibay might know if this is possible.

  1. Is it ok that if the number of repeats is 1 that I set the uncertainty to NaN?

Yes I think the reported uncertainty which we currently have as the std of the repeats, can be NaN or 0.0, but we should also report the estimator error for each repeat in the computational_results.json.

alchemical_archives = [_load_archive(arch) for arch in archive]
network_obj = alchemical_archives[0].network

# Verify all archives have the same network

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do all archives have to use the same network I think it would be fine to let them be different and just combine the results for example if we have run all of the jacs systems and have an archive per system we should be able to pass them all to this script.

Comment on lines +409 to +414
# Write to temporary file for loading
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as tmp:
tmp.write(json_content)
tmp_path = tmp.name
else:
tmp_path = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can skip this step and pass the raw json to the from_json method.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't notice this one after fixing it in the generate script. Fixed now

Comment on lines +27 to +29
openfe_version: TODO
openmm_version: TODO
openff_toolkit_version: TODO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add versions.

Comment on lines +47 to +48
doi: TODO add DOI
archive_provider: TODO add archive provider

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add zenodo data.

@@ -0,0 +1,83 @@
#!/usr/bin/env python

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice this is a great example of the API working!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to save this to scripts as an example rather than in the results dir for this submission to try and keep the folders uniform?

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