Conversation
…ource code for bugs
benmwebb
left a comment
benmwebb
left a comment
There was a problem hiding this comment.
Looks generally good to me. I made a couple of comments which you can address if you like (not deal breakers). But you do need to rebase your branch against current main (or merge main into your branch) as your branch won't merge cleanly right now, likely due to changes I've made in the meantime.
|
|
||
| def sort_ses(self, ses): | ||
| # Given a list of structural elements, sort them by increasing first residue | ||
| res = sorted([(s.get_first_residue_number(), s) for s in ses], key=lambda x: x[0]) |
There was a problem hiding this comment.
Possibly more efficient to use res = sorted(((s.get_first_residue_number(), s) for s in ses), key=lambda x: x[0]) here (i.e. give sorted a generator expression (...) rather than a list [...]). That way it doesn't need to construct an intermediate list in memory. (Probably moot in this case though since to sort it it will likely coerce to a list anyway, but it's a better habit to get into.) Also key=operator.itemgetter(0) is perhaps cleaner.
| m = IMP.Model() | ||
| ###################################### | ||
| DATADIR = os.path.abspath(os.path.join(os.path.dirname(__file__), 'input')) | ||
| pdbfile = os.path.join(DATADIR, pdb_name) |
There was a problem hiding this comment.
Replace these two lines with pdbfile = self.get_input_file_name(pdb_name). IMP already has a utility function to get test inputs; no need to reinvent the wheel here.
| print("SECR:", se[0].get_start_res(), se[1].get_start_res(), r.get_number_of_residues(), r.get_model_distance(), r.get_max_distance(), r.unprotected_evaluate(None)) | ||
| # maximum distance between two SSEs should not be zero, which means no residues between these two secondary structure elements, this should be penalized by having a high score | ||
| if round(max_dist) == 0.0: | ||
| self.assertGreater(r.unprotected_evaluate(None), 1000.0) |
There was a problem hiding this comment.
Probably fine here, but unprotected_evaluate is, as the name implies, unprotected and doesn't do the usual pre-evaluation setup (such as ensuring rigid body coordinates match their reference frame). r.evaluate() is the corresponding 'protected' variant which is usually what you want in Python.
| m = IMP.Model() | ||
| ###################################### | ||
| DATADIR = os.path.abspath(os.path.join(os.path.dirname(__file__), 'input')) | ||
| pdbfile = os.path.join(DATADIR, pdb_name) |
There was a problem hiding this comment.
Same thing here with get_input_file_name.
2 participants
I made a few changes following @benmwebb suggestions.