Rename variable names in NNP factory class#283
Conversation
chrisiacovella
left a comment
chrisiacovella
left a comment
There was a problem hiding this comment.
We probably should rename "model.py" to "potential.py". Are you using an IDE where it will automatically refactor? If not I can do this and push a change.
|
|
||
| from openff.units import unit | ||
|
|
||
| from modelforge.train.training import ModelTrainer |
There was a problem hiding this comment.
Were you going to change the name of ModelTrainer to PotentialTrainer?
There was a problem hiding this comment.
That actually makes more sense, and I am changing it now.
| inference_neighborlist_strategy: str = "verlet", | ||
| verlet_neighborlist_skin: Optional[float] = 0.1, | ||
| ) -> Union[Potential, JAXModel, pl.LightningModule]: | ||
| ) -> Union[Potential, JAXModel, pl.LightningModule, ModelTrainer]: |
There was a problem hiding this comment.
Same here, were we going to change this to PotentialTrainer?
|
Lint is failing; might just need to check line lengths in model.py, training.py, and test_training.py |
@chrisiacovella Are you saying to rename the file and update with git? I can do that, or I can just wait until you push a change to refactor class names inside. For simplicity, could you push a commit of renaming files (if I understood correctly)? |
Yeah I will check. I did rename "self.model" to "self.potential_training_adapter" as they are actually hodling a TrainingAdapter objects which may result in some lines being too long. |
…on in helper_functions.py to stop writing to the main test directory.
…me functions writing to cache
…riting to the temp dir
…riting to the temp dir
chrisiacovella
left a comment
chrisiacovella
left a comment
There was a problem hiding this comment.
Major change was renaming models.py to potential.py to be consistent with our recently agreed upon naming scheme. While at it, I fixed some of the formatting issues in test_training, and fixed a few of the tests in test_potentials.py that I missed in terms of making sure all tests right to temp directories.
Since the names have changed, the .rst files in the docs still need to be updated to reflect the naming change. I think we can merge after the docs are updated.
wiederm
left a comment
wiederm
left a comment
There was a problem hiding this comment.
Great changes @MarshallYan and @chrisiacovella !
wiederm
added
enhancement
|
It looks like the only thing that still needs to be addressed is updating the .rst files in docs. |
chrisiacovella
left a comment
chrisiacovella
left a comment
There was a problem hiding this comment.
This looks good. Let's get this merged.
4 participants
Pull Request Summary
Rename variables to distinguish among "Potential", "ModelTrainer", and "TrainingAdapter" objects in models.py, training.py, test_models.py, and helper_functions.py.
Key changes
Notable points that this PR has either accomplished or will accomplish.
ModelTrainerobjectsPotentialobjectsTrainingAdapterobjectsModelTrainerobject or aPotentialobject (will be refactored and separated in another PR)Associated Issue(s)
Pull Request Checklist