Fix issue #100

[chikitng]

No description provided.

[fjclark]

Thanks @chikitng, this looks like a good start! From a quick skim, here are a few initial suggestions and comments:

• I'm not sure where the best place for this to live is (tutorials or notebooks). There seems to be a bit of overlap between these. @bieniekmateusz / @BenCree any suggestions for which is best / what's the intention for each? Should we add a quick section to Home directing users to each?
• I couldn't run this out of the box because crem wasn't installed. I see that it's only available through pypi, so can't be included as a dependency on the conda package. Could you include a section at the top of the notebook which instructs the user to install this or just installs it? e.g.

! pip install crem

• I couldn't run it after installing crem as the paths weren't correct e.g. scaffold = Chem.SDMolSupplier('5R83_core.sdf')[0] (should have been pointing to the sarscov2 directory). Could you please ensure that all the paths are correct and that this runs out of the box with a fresh install?
• It would be great if you could standardise formatting a bit with the other notebooks and tutorials, for example check out the basic full tutorial -- could you please add and overall title, bullet point summary, overview section, authors, etc? Thanks!
• I think the wording could be improved in a few pages to make this easier to understand. There are places where there are capitalised words in the middle of sentences, for example. It might be worth even running this through an LLM and asking it to find mistakes and unclear phrasing before fixing yourself.
• I like to auto-format and clean my notebooks before I commit -- here are examples of how to do this . This makes the code cleaner and also means that git diffs are much cleaner and only include the code changes you make, which also makes reviewing easier!

Please let me know if any of that is unclear or you need a hand with anything -- happy to jump on a call or slack. Thanks very much!

[fjclark]

Assigning @BenCree -- thanks for volunteering in #100!

[bieniekmateusz]

Well done,

@fjclark You can add a pip package to the conda environment, see https://stackoverflow.com/a/74370044/1218179

Minor comment: I would clear the notebook and have it without outputs. Otherwise it is almost like a binary file in git, which will create issues with diff, tracking and the size of the repository.

[fjclark]

@fjclark You can add a pip package to the conda environment, see https://stackoverflow.com/a/74370044/1218179

Thanks! My understanding though is that it's bad practice to pip install things in conda forge recipies so it would be tricky to make crem available with mamba install -c conda-forge fegrow -- or is there a way to do this?

[bieniekmateusz]

You're right, pip might overwrite some conda installed dependencies, but other than releasing the package on conda, not much you can do if you have a new feature that relies on it, you can control the risks by pinning packages and their dependancies to the tested ones (see pip compile) if you want a full control.

I believe for an open source package with CI, with your further testing, the risks are acceptable.

Manual installation with pip has the same problems.

[bieniekmateusz]

Maybe worth trying to install crem and excluding the overlapping dependencies that conda already installed.

[fjclark]

I just had a look at this, and between running mamba create -n fegrow -c conda-forge fegrow and pip install crem, crem is the only package which appears. It pretty much only requires rdkit.

To make sure I understand, is your recommendation:

• Add a pip installation of crem to the end of the conda yaml file, so that users who install from environment.yml will have it (and will work for CI) but users who install from conda forge will not
• Keep a manual installation/ warning for users at the top of the notebook in case users have installed though conda forge?

Thanks.

[fjclark]

(As a side-note, I've found https://pixi.sh/latest/ to be great for properly handling interdependencies between conda/ pypi deps)

[fjclark]

Also, what's the difference between tutorials and notebooks and which do you recommend for this @bieniekmateusz? Thanks

[bieniekmateusz]

Keep a manual installation/ warning for users at the top of the notebook in case users have installed though conda forge?
I guess so. In the meantime, can we release crem on conda-forge? I can check later how complex crem is.

Sorry, not sure about the notebooks and tutorials, it is a bit of a mess.

[fjclark]

I guess so. In the meantime, can we release crem on conda-forge? I can check later how complex crem is.

Thanks. It seems to pretty much only need rdkit and is versioned, so I don't think this should be too hard. However, given that @chikitng's changes don't introduce any features to FEGrow which use crem, rather just a notebook showing how to use crem in combination with FEGrow, I guess it's not completely clear if it should be a core dep or not? Either way a conda-forge package would be nice though.

Sorry, not sure about the notebooks and tutorials, it is a bit of a mess.

No worries, thanks. Should we consolidate/ merge them all into one place with a summary? Any comments on this @djcole56? Thanks.

[djcole56]

I think just leaving crem as an optional dependency is fine (from memory it also needs a library download), with a recommendation to the user for how to install it.

Just keeping one of notebooks/tutorials would be an improvement.