Skip to content

Conversation

@dwhswenson
Copy link
Member

@dwhswenson dwhswenson commented Mar 4, 2022

This majorly improves the performance of the _remove_constraints method. We can probably switch to using the fix_constraints flag in the notebook now. Actually, we can probably drop the tqdm progress bars, too.

4 characters... this is a "making chalk mark on generator" PR if ever I've made one!

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

Binder 👈 Launch a binder notebook on branch OpenFreeEnergy/ExampleNotebooks/rbfe-perf-fix-constraints

@IAlibay
Copy link
Member

IAlibay commented Mar 4, 2022

So I somewhat specifically left optimisations alone because I felt it was better for us to optimise when we push this into the OpenFE toolkit than deviating too much from the original Perses source early on (so for example there's a point here to be made that you don't really need to loop through all the atoms because only core atoms should change bond lengths).

I get it's just to speed up the membership check, but essentially what I'm getting at is, do we want to wait on these things until we do our own implementation?

@IAlibay
Copy link
Member

IAlibay commented Mar 4, 2022

I'm probably being overcautious given how straightforward a change this is (i.e. it's not like it's really changing behaviour or anything).

Let's try to keep track on changes here though, just so we don't deviate too far away without tests (not that there aren't already plenty of parts of this that have been hacked together 🙄 ).

I've updated the notebook and the warning messages accordingly. Thanks @dwhswenson !

@IAlibay
Copy link
Member

IAlibay commented Mar 4, 2022

Not waiting on tests given they currently fail.

@IAlibay IAlibay merged commit 3476936 into master Mar 4, 2022
@IAlibay IAlibay deleted the rbfe-perf-fix-constraints branch March 4, 2022 12:14
@dwhswenson
Copy link
Member Author

I get it's just to speed up the membership check, but essentially what I'm getting at is, do we want to wait on these things until we do our own implementation?

Yeah, I'm not really going through looking for optimizations. More that I was looking over the code, and the use of tqdm caught my eye (as a bad thing in library code). Then I noticed the comments that it was slow. Then I looked at the code and said "this shouldn't be slow." Then I saw the problem, and confirmed the fix locally. Figured it was better update the repo than to try to locally track fixes I've identified.

At this point I'm mostly thinking about things where reimplementation should be structured in a more modular way than existing, and trying to get a handle on what that modular structure should be.

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