Skip to content

Conversation

@richardjgowers
Copy link
Contributor

introduce GroundState object

instead all measurements are relative to a GroundState

Description

Provide a brief description of the PR's purpose here.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • TODO 1

Questions

  • Question1

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #84 (43d78c1) into main (69ed025) will increase coverage by 2.76%.
Report is 8 commits behind head on main.
The diff coverage is 96.61%.

Additional details and impacted files

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 lgtm, I really like this idea.

Only comments are:

  1. A bit more documentation on behaviour would be good
  2. It's hard to tell if this is good because I like it, I'd be ok with merging this quickly, but if we can have someone else gloss over the idea afterwards that'd be grand.

expt_block = False
calc_block = False

ground = GroundState()
Copy link
Member

Choose a reason for hiding this comment

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

worth documenting that ground state is assumed to be the same for all inputs for this method?

Copy link
Member

Choose a reason for hiding this comment

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

Also is there a unique string we could give it so it hashes differently?

>>> kJpm = unit.kilojoule_per_mole
>>> experimental_result1 = AbsoluteMeasurement(label="CAT-13a", DG=-8.83 * kJpm, uncertainty=0.10 * kJpm)
>>> experimental_result2 = AbsoluteMeasurement(label="CAT-17g", DG=-9.73 * kJpm, uncertainty=0.10 * kJpm)
>>> g = GroundState()
Copy link
Member

Choose a reason for hiding this comment

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

Could you introduce the idea of label here so that folks can understand that it can be set to different states that hash differently?

@jchodera
Copy link
Contributor

jchodera commented Jul 4, 2023

Is "Ground state" a reference state with absolute free energy defined to be zero? If so, maybe ZeroState or ReferenceState is more clear?

DiffNet permits multiple absolute free energy measurements of multiple states relative to that zero. Will it still be possible to have multiple absolute measurements?

@richardjgowers
Copy link
Contributor Author

@jchodera GroundState is a reference state, TrueGround is the theoretical zero point, and is a special case of GroundState. "Absolute" measurements are against one of these two types of points.

For example, MLE would first place computed ligands against an arbitrary MLE GroundState, then next draws a measurement between that GroundState and the GroundState of the experimental (absolute) measurements. The derived absolute computational values are then the sum of the edge to the arbitrary ground and the edge to the ground of the experimental points.

flowchart TD
  G[Experimental GroundState] -- experiment --> A[experimental A];
  G  -- experiment --> D[experimental B]; 
  H[MLE GroundState] --> B[computational A];
  H --> C[computational B];
  B -- RBFE --> C;
  G -- derived --> H;
  Z[True Ground] -- ??? --> G;
Loading

@IAlibay
Copy link
Member

IAlibay commented Jul 5, 2023

Is there really a need for something like TrueGround? If I'm being only slightly cynical (more likely just a realist), the standard deviation around even high quality experiment is such that you are really just setting your ground state to a given set of experimental conditions at a given time in space.

edit: I think my main issue is with the word True maybe just ReferenceGround would work? Then folks could assign a reference ground state, but not necessarily assert that this is the sole "truth".

@richardjgowers
Copy link
Contributor Author

this diagram is better actually:

flowchart TD
  G[Experimental GroundState] -- experiment --> A[ligand A];
  G  -- experiment --> B[ligand B]; 
  H[MLE GroundState] -- MLE --> A;
  H -- MLE --> B;
  A -- RBFE --> B;
  G -- derived --> H;
  Z[True Ground] -- assumed 0 --> G;
Loading

@IAlibay TrueGround is maybe unnecessary? The way some sort of FEMap.get_absolute('ligand A') would work would be to pathfind from TrueGround to ligand A, which would be the sum of three edges (one of which is zero, so could collapse it away?).

richardjgowers and others added 19 commits September 1, 2023 11:59
introduce GroundState object

instead all measurements are relative to a GroundState
allows path finding using nx while preserving the sign of measurements
optional, defaults to 298.15 K
* convert IC50 to DG

* Change input from float to unit.Quantity, fix error calculation

* Add test for from_experiment function

* Add parametrize fixture

* Add test for error messages

* remove .idea files and new example ipynb

* remove duplicate test file

* Fix pep8 requirements

* fix pep8 requirements

* fix pep8 requirements - line length

* fix typo unit name
easily add experimental/computational values
can't use pipes for OR, use Union
@richardjgowers richardjgowers merged commit c7f934c into main Sep 1, 2023
@richardjgowers richardjgowers deleted the use_groundstate branch September 1, 2023 12:03
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.

5 participants