Skip to content

add findSurfaceResiduesListCharged.py #132

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

Closed
wants to merge 5 commits into from

Conversation

JarrettSJohnson
Copy link
Contributor

Adding this script on behalf of Teddy Warner from MIT.

Script page on the Wiki: https://pymolwiki.org/index.php/FindSurfaceResiduesListCharged

@JarrettSJohnson JarrettSJohnson requested a review from speleo3 March 13, 2023 21:13
@JarrettSJohnson
Copy link
Contributor Author

Thanks @speleo3 , I forwarded the feedback to Teddy. He replied with some changes and a new file/API: https://pymolwiki.org/index.php/FindSurfaceCharge

Copy link
Contributor Author

@JarrettSJohnson JarrettSJohnson left a comment

Choose a reason for hiding this comment

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

Doesn't run properly if there are multiple objects in the selection (e.g. multiple objects in the workspace and you run the command without any provided arguments). Perhaps this should first detect the objects in the selection first and provide the charge for each object surface.

cmd.iterate(selName, "exposed.add((resv))", space=locals())
cmd.delete(selName)

selNameRes = cmd.get_unused_name("exposed_res_")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused variable?

cmd.iterate(allRes, "first.add((resv))", space=locals())
cmd.delete(allRes)

selNameRes = cmd.get_unused_name("exposed_res_")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused variable?


first=sorted(first)[0] #firstRes
#gets all charged amino acids on the surface
reslist= []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reslist appended, but never used?

return selName


def findSurfaceCharge(pH=7.0, folded=True, selection="all", cutoff=2.5):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be helpful to also return the charge so that it's easier for the caller to obtain.

Comment on lines 67 to 77
cutoff = float(cutoff)

selName = findSurfaceAtoms(selection, cutoff)

exposed = set()
cmd.iterate(selName, "exposed.add((resv))", space=locals())
cmd.delete(selName)

selNameRes = cmd.get_unused_name("exposed_res_")

exposed=sorted(exposed) #list of exposed residues
Copy link
Contributor Author

Choose a reason for hiding this comment

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

readability: would probably pull out to another function

get_exposed_residues(selection: str, cutoff: float) -> set:

or similar.

Comment on lines 84 to 91
first = set()
allRes = findSurfaceAtoms(selection, 0)
cmd.iterate(allRes, "first.add((resv))", space=locals())
cmd.delete(allRes)

selNameRes = cmd.get_unused_name("exposed_res_")

first=sorted(first)[0] #firstRes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

readability: might be more readable to pull this out into its own func.

get_first_residue(selection: str) -> int:

Copy link
Contributor

@speleo3 speleo3 left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

Some optional improvements:

  • add type hints
  • auto-format code (yapf, black, ...)
  • add module docstring which points to the wiki page
  • folded argument is not really needed since the user can pass cutoff=0

Tests would be nice, but I understand that there is no infrastructure in place in the repo.

@JarrettSJohnson
Copy link
Contributor Author

Merged. As separate commit 4fcf977 since Github doesn't seem to specific commit author to someone else if one performs a squash merge.

@JarrettSJohnson JarrettSJohnson deleted the findSurfaceResiduesListCharged branch April 27, 2023 13:26
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.

3 participants