ersilia-os/olinda

Refactoring files for better modularity

Closed this issue · 3 comments

GemmaTuron commented

There are some small changes that will help keep Olinda more modular for future developments. We have had a look at the code with @miquelduranfrigola and here are our suggestions:

  • Rename the folder tools to utils, for ersilia's convention
  • Keep all utils related to zairachem models inside a folder in utils/zairachem
  • Rename the precalc_descriptors.py to precalculations.py, since there is also Mellody for example which is not a descriptor
  • Move the bunch of functions from the utils.py file to the utils folder under the necessary modules, so that it does not become a long messy script. For example, if it is a zairachem needed, it should only be under the Zairachem folder. Otherwise, when trying to import a function from utils.py Olinda will crash if zairachem is not installed (even if it is not needed)
  • Double check the function precalc_descriptors in the file models/zairachem.py. Is it redundant with what is available in the utils file and could it be reused from there simply?
  • The file distillation.py does not even have a class, would be good to have it
JHlozek commented

I've implemented all of these suggestions.

For the second last point, the precalc_descriptors function (now in utils/zairachem/zairachem.py) is used during the descriptor step at runtime to copy in place the cached descriptors and prevent their calculation at runtime. The precalculations.py file on the other hand calculates the descriptors with the required format to be stored in AWS (i.e. it automates the descriptor calculation of the folds of the grover reference library). Seeing as it is not used at runtime, perhaps precalculations.py should be moved to a separate scripts folder?

The distillation.py was the original main script and I've included a Distiller class with the main distill() function. The remaining functions should probably also be converted to an object oriented paradigm but I think this is lower priority for now.

miquelduranfrigola commented

Thank you @JHlozek ! @GemmaTuron let's have a look.
About the precalculations.py file being moved to a scripts folder, I think this makes sense in principle

GemmaTuron commented

thanks @JHlozek!

Could you open a PR and I'll take the rest of the reformatting steps? I think we need to get to the point where Olinda does not install ZairaChem to avoid redundancy, but let's take it bit by bit. I'll close this issue meanwhile