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

Leaflet sorter speed up and shell selection bug fix #102

Conversation

JesseSandberg
Copy link
Contributor

@JesseSandberg JesseSandberg commented Dec 6, 2024

Description

When running polarDensityBin the leaflet sorter would run on all lipids in the system, even those outside of the zone of analysis. After this PR, the user can select whether leaflet sorting will only occur for lipids within Rmax or for all lipids.

In the process, we discovered a bug (issue #104). If the user specified an Rmax that was evenly divisible by dr, then polarDensityBin would analyze past Rmax by one shell. If the user specified an Rmax that was not evenly divisible by dr, then polarDensityBin would happily accept that choice without balking (but density enrichment analysis would be affected in the outermost shell downstream).

We also found that expected density was being measured on start_frame rather than end_frame (issue #107).

We also found a minor error when no lipids are detected within the shell (issue #110).

After this PR, shells will no longer exceed Rmax when Rmax is evenly divisible by dr. Additionally, polarDensityBin will give an error message and quit if Rmax is not evenly divisible by dr. This fix is agnostic to whether the user uses ints or floats in their config file. Expected density will be measured on end_frame.

Implementation of auto-rescaling dr (issue #105) will occur in a future PR

Usage Changes

  • User has access to restrict_leaflet_sorter_to_Rmax parameter (default is 0).
  • User will receive an error message if they specify Rmax not evenly divisible by dr.

Todos

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

  • Leaflet sorting can either sort all lipids or only sort lipids within Rmax
  • If Rmax is evenly divisible by dr, shells will no longer exceed Rmax
  • Expected density measured on end_frame
  • User receives error message f Rmax is not evenly divisible by dr.
  • User receives error message if restrict_leaflet_sorter_to_Rmax is not 1 or 0 (default is 0)
  • Bug fix: empty shell no longer causes error.

Pre-Review checklist (PR maker)

  • Ensure no run-time errors
  • Test with variety of Rmax and dr options
  • Test with variety of restrict_leaflet_sorter_to_Rmax options
  • Run same analysis twice - once prior to change and once after change - and then vimdiff the outputs to ensure they are identical
  • Run same analysis twice - once prior to change and once after change - and ensure that resulting delta Gsite is the same at the very end (perform end-to-end testing)
  • Include test in the DTA_Testing repo (here)

Review checklist (Reviewer)

  • No missed "low-hanging fruit" that would substantially aid readability.
  • Any "high-hanging" or "rotten" fruit is added to the issues list.
  • I understand what the changes are doing and how
  • I understand the motivation for this PR (the PR itself is appropriately documented)

Status

  • Ready for review

@JesseSandberg JesseSandberg self-assigned this Dec 6, 2024
@JesseSandberg JesseSandberg linked an issue Dec 6, 2024 that may be closed by this pull request
@JesseSandberg
Copy link
Contributor Author

closed because I found aberrant behavior with lipids that were only partially enclosed in the shell created by Rmax

@JesseSandberg JesseSandberg reopened this Dec 9, 2024
@JesseSandberg JesseSandberg changed the title leaflet sorter now only runs on lipids within rmax DRAFT: leaflet sorter now only runs on lipids within rmax Dec 9, 2024
@JesseSandberg JesseSandberg removed the request for review from gbrannigan December 9, 2024 14:34
@JesseSandberg JesseSandberg marked this pull request as draft December 9, 2024 14:34
@JesseSandberg JesseSandberg linked an issue Dec 9, 2024 that may be closed by this pull request
@JesseSandberg JesseSandberg changed the title DRAFT: leaflet sorter now only runs on lipids within rmax leaflet sorter speed up and shell selection bug fix Dec 9, 2024
@JesseSandberg JesseSandberg changed the title leaflet sorter speed up and shell selection bug fix DRAFT: leaflet sorter speed up and shell selection bug fix Dec 9, 2024
@JesseSandberg JesseSandberg changed the title DRAFT: leaflet sorter speed up and shell selection bug fix Leaflet sorter speed up and shell selection bug fix Dec 10, 2024
@JesseSandberg JesseSandberg marked this pull request as ready for review December 10, 2024 14:09
@JesseSandberg
Copy link
Contributor Author

Note from convo today: This PR will be reviewed when I have completely replicated analysis on the nachr project from end to end with this branch and gotten the same results out as previously.

@JesseSandberg JesseSandberg marked this pull request as draft December 16, 2024 22:30
@JesseSandberg
Copy link
Contributor Author

As discussed in nougat meeting, I have re-run a DTA calculation from a separate project and confirmed that the site delta G does not change when these changes are made.

@JesseSandberg
Copy link
Contributor Author

As discussed in the lab, I have created a separate repo - DTA_Testing - that now houses any testing notebooks and materials created.

set divisibility_test [expr [expr int([expr $params(Rmax) * 10000])] % [expr int([expr $params(dr) * 10000])]]
if {$divisibility_test != 0} {
error "Rmax must be evenly divisible by dr."
}
Copy link
Member

@gbrannigan gbrannigan Jan 23, 2025

Choose a reason for hiding this comment

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

@JesseSandberg

This code is more complicated than it needs to be. Do a simple (i.e. double($Rmax) % $dr) modulo to get the remainder, and then compare the remainder against a small tolerance (not 0!).

Do not make the tolerance a magic number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code has been modified per discussion on slack. Thank you!

Copy link
Member

@gbrannigan gbrannigan left a comment

Choose a reason for hiding this comment

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

I have left two comments, please address.

@gbrannigan gbrannigan merged commit 8bc8ccf into master Jan 24, 2025
@gbrannigan gbrannigan deleted the 98-leaflet-sorting-should-only-select-lipids-within-zone-of-analysis branch January 24, 2025 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants