Skip to content

Add a PyRosetta unit test for BindCraft - #753

Open
roccomoretti wants to merge 5 commits into
RosettaCommons:mainfrom
roccomoretti:bindcraft_python_test
Open

roccomoretti wants to merge 5 commits into
RosettaCommons:mainfrom
roccomoretti:bindcraft_python_test

Conversation

@roccomoretti

Copy link
Copy Markdown
Member

This doesn't test all of BindCraft, it just attempts to test the PyRosetta portions of it, to make sure we don't have future regressions on the use. (Like what was seen with PR #749)

One limitation is the lack of dalphaball. BindCraft uses it, but the I do not think it's easy to use on the Rosetta test server. It's turned off for this test, and hopefully not exercising the dalphaball-related code doesn't cause too many issues.

This doesn't test all of BindCraft, it just attempts to test the PyRosetta portions of it,
to make sure we don't have future regressions on the use.

One limitation is the lack of dalphaball.

@lyskov lyskov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me—thank you for adding this, @roccomoretti! Two small notes:

  • Since the test writes files to a temp directory, it would be best to follow the PyRosetta test convention and explicitly change the working directory.
  • The init call is currently somewhat buried. I suggest moving it to the top, as in our other tests, so it is immediately clear that a constant seed is being used.

I would recommend adding the following near the beginning of the file to address both:

pyrosetta.init(extra_options="-constant_seed -out:mute all")  # WARNING: '-constant_seed' is for testing only!
import os
os.chdir(".test.output")

@roccomoretti

Copy link
Copy Markdown
Member Author

I'm not entirely following the point of changing the directory, but I should be able to change that.

Regarding the location of the init(), that's in the setUp function of the test class. It is near the top, provided you ignore the transclusion.

Note that this is purely intended as a test case -- in contrast to other tests which might have been intended for "examples", no one should be using this file as a example code for their own use case.

@lyskov

lyskov commented Aug 25, 2026

Copy link
Copy Markdown
Member

I'm not entirely following the point of changing the directory, but I should be able to change that.

-- This is in case protocol write some files into current directory. I understand that if everything goes well it should not happened but we had cases in the past that extra files was dumped in case of errors or other "random" event. So this is more of precautionary measure. So if you do not mind including it it would be great!

Regarding the location of the init(), that's in the setUp function of the test class. It is near the top, provided you ignore the transclusion.

Note that this is purely intended as a test case -- in contrast to other tests which might have been intended for "examples", no one should be using this file as a example code for their own use case.

-- Sure Rocco, this sounds reasonable to me. I am fine of keeping init line location as it is now.

@lyskov lyskov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, - thank you @roccomoretti !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants