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

Sampler #17

Merged
merged 33 commits into from
Sep 18, 2024
Merged

Sampler #17

merged 33 commits into from
Sep 18, 2024

Conversation

fgrunewald
Copy link
Collaborator

Initial draft for a sampler class that takes fragments and makes random molecules according to bonding probabilities

Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Some small comments, looks mostly fine to me.

cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

I like most of the changes, but I'm not happy with how you seed the random generator. You should seed it only once, at the start of your process. Otherwise your seeded random will not be random enough.
You can also, for reproducibility, generate a seed if None is given (time.time_ns()) and log that somewhere the user can find it.

cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/cgsmiles_utils.py Show resolved Hide resolved
cgsmiles/cgsmiles_utils.py Outdated Show resolved Hide resolved
cgsmiles/pysmiles_utils.py Show resolved Hide resolved
cgsmiles/pysmiles_utils.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
@fgrunewald fgrunewald requested a review from pckroon September 10, 2024 16:24
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

For the Sampler examples, I'm not super clear on what the difference between *reactivities and *probabilities is. Also, for some examples you do from_string, for other from_fragment_string. Is this intentional?
Lastly, does it make sense to completely replace the terminal logic with the fragment reactivities?

cgsmiles/resolve.py Show resolved Hide resolved
cgsmiles/resolve.py Show resolved Hide resolved
cgsmiles/resolve.py Show resolved Hide resolved
cgsmiles/resolve.py Show resolved Hide resolved
cgsmiles/resolve.py Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
@fgrunewald
Copy link
Collaborator Author

@pckroon All reactivities are probabilities. Reactivities is used when the decision is based on a bonding descriptor. I've added a clarification in the docs.

they should all be from_fragment_string but some examples were older from before

Not entirely because at the CG level branch termination cannot be determined by the bonding descriptor when no end-group is added. Otherwise, yes one could do as suggest. Of course one other option is to simply define an empty or fake terminal. But not sure that makes it easier.

@fgrunewald fgrunewald requested a review from pckroon September 12, 2024 15:28
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification. The docstring(s) also help me :)
Still found a typo though.

As for the terminals, I don't have a super strong preference for one or the other. I just know from experience that the more complex an API is, the harder it is to use. It requires more mental space to keep track of all the arguments, their different formats/types/conventions, and how it all interacts.

cgsmiles/resolve.py Outdated Show resolved Hide resolved
cgsmiles/resolve.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Outdated Show resolved Hide resolved
@fgrunewald fgrunewald requested a review from pckroon September 18, 2024 10:14
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Still found some small points :)

cgsmiles/sample.py Outdated Show resolved Hide resolved
cgsmiles/sample.py Show resolved Hide resolved
cgsmiles/sample.py Show resolved Hide resolved
Co-authored-by: Peter C Kroon <[email protected]>
@fgrunewald fgrunewald requested a review from pckroon September 18, 2024 12:14
@fgrunewald fgrunewald merged commit 2aaf60b into master Sep 18, 2024
6 checks passed
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.

2 participants