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

Improving Energy and Force Methods #42

Closed
shenoynikhil opened this issue Mar 8, 2024 · 4 comments
Closed

Improving Energy and Force Methods #42

shenoynikhil opened this issue Mar 8, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@shenoynikhil
Copy link
Collaborator

shenoynikhil commented Mar 8, 2024

Currently, energy and force methods per dataset are given by a list of values. In cases where an energy method does not have forces, the value does not exist in the force method list, for instance,

__energy_methods__ = [method_1, method_2, method_3]
# say method_1 does not give forces
__force_methods__ = [method_2, method_3]

This can (possibly) cause complications the same index in the two list corresponds to 2 different things causing indexing during forward pass tricky.

Alternative solution

Have an energy method list and a boolean list for forces which specify if its present or not

# using the same example as above
__energy_methods__ = [method_1, method_2, method_3]
__force_methods__ = [False, True, True]

This will definitely cause multiple downstream complications, therefore needs to be done carefully.

CC. @FNTwin @prtos @S-Thaler

@shenoynikhil shenoynikhil added the enhancement New feature or request label Mar 8, 2024
@FNTwin
Copy link
Collaborator

FNTwin commented Mar 8, 2024

Agree with this improvement as the force mask is already a bit strange and it makes lot of redundant coding down the line.
We have the same example (1) on some ANI dataset and it could be even worse as we can also have something along the line of

__energy_methods__ = [method_1, method_2, method_3, method_4]
# say method_1 does not give forces
__force_methods__ = [method_1, method_3]

By just using a mask on __force_methods__ we will improve a lot in my opinion.

Bonus point if we swap the indexing of the forces that we have currently as they are a little bit strange to index.

@prtos
Copy link
Contributor

prtos commented Mar 8, 2024

I understand the point and agree with the proposal. However we must introduce a force_mask attribute and make force_methods a property

# using the same example as above
__energy_methods__ = [method_1, method_2, method_3]
__force_mask__ = [False, True, True]

@FNTwin
Copy link
Collaborator

FNTwin commented Mar 8, 2024

In development here: https://github.com/OpenDrugDiscovery/openQDC/tree/force_mask
PR on hold here: #43

Add the following:

  • __force_mask__ as private attribute
  • add energy_methods,force_mask , force_methods property methods
  • __force_methods__ are now based on energy_methods,force_mask
  • Add the correct __force_mask__ to every dataset
  • Automatically initialize __force_mask__ to self.__class__.__force_mask__ = [False] * len(self.energy_methods) if empty (aka no forces)

@shenoynikhil shenoynikhil mentioned this issue Mar 8, 2024
3 tasks
@FNTwin
Copy link
Collaborator

FNTwin commented Mar 20, 2024

Closing as merged. Added __force_methods__ property for retrocompatibility

@FNTwin FNTwin closed this as completed Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants