michellab / BioSimSpaceTutorials

25 stars 10 forks source link

Questions and todo's as they arise around the FEP tutorial #16

Closed ppxasjsm closed 1 year ago

ppxasjsm commented 2 years ago

I am using this issue to help with merging this and this set of FEP tutorials. I'll keep updating to dos adding questions etc.

Questions:

Questions on FEP tutorial structure:

Overall to do:

jmichel80 commented 2 years ago

We keep the ABFE tutorial as part of the 4th FEP tutorial?

Yes, we will also explain how this is accessed using the 'Sandpit' functionality and what is the purpose of having a sandpit.

Are we archinving keeping Jenke's tutorial on FEP?

Yes

Do we need a copy of the source of freenrgyworkflows in the tutorial? This seems very dodgy from a coding perspective and can lead to diverging repos

And

Replacement` of freenrgyworkflows with arsenic (or whatever the latest name now is) will this happen? Has this happened?

Agreed, @anna had to do this due to bugfixes that are not on https://github.com/michellab/freenrgworkflows How close to usable is https://github.com/OpenFreeEnergy/cinnabar ? We were hoping to have removed the freenrgworkflows dependency already but I haven't heard much about progress from OpenFE on that end. Maybe I can flag this at the next OpenFE TAC meeting but in the meantime we could go ahead and check if cinnabar is usable.

Are we offering some link to azure or co-lab to be able to run these directly from the tutorial repository?

@chryswoods and I are working on a solution for this as notebook.biosimspace.org will stop working next month. It will definitely possible to run the tutorial suite remotely.

Should the ccpbiosim repo be moved to the ccpbisim gitbub?

Yes good idea !

Should` the headers be made uniform and the specific reference to ccpbiosim training week removed?

Yes, this should be standalone and ready for consumption online in an asynchronous manner

Should` all tutorial jupyter notebooks have uniform headers and also clearly state contributions of people writing the tutorials?

That would be nice, and would help figure out how to optimise the list of authors in future updates of the manuscript.

Should 04_FEP contain 3 separate tutorials? 01_Intro_to_alchemy, 02_advanced_RBFE and 03_ABFE?

Yes since they covered different aspects of FEP. This should map to subsections of 04.

ppxasjsm commented 2 years ago

Thanks for all the answers! I'll start working on some of this.

@annamherz could you have a look at how usable cinnebar is? I don't think I'll have time, but it would be great to have this incorporated if possible and not use dodgy fixes of freenrgyworkflows.

ppxasjsm commented 2 years ago

Would it make sense to add authors also as metadata to notebooks?

Like such:

{
"ipub": {
  "titlepage": {
  "author": "Authors Name",
  "email": "authors@email.com",
  "supervisors": [
    "First Supervisor",
    "Second Supervisor"
  ],
  "title": "Main-Title",
  "subtitle": "Sub-Title",
  "tagline": "A tagline for the report.",
  "institution": [
    "Institution1",
    "Institution2"
  ],
  "logo": "path/to/logo_example.png"
  }
  }
}
ppxasjsm commented 2 years ago

I've moved the ccpbiosimworkshop material to here:

https://github.com/CCPBioSim/BSS_FEP_workshop_2022

Not sure if you want to keep both repos, but currently the content is identical.

ppxasjsm commented 2 years ago

I will be doing all work on this branch: 04_fep_integration

ppxasjsm commented 2 years ago

@jmichel80 and @annamherz, if cinnebar cannot be integrated yet, can we add the bug fixes to freenrgworkflows and remove the dependency? I also don't know what has been happening as I was way too busy with some other things.

annamherz commented 2 years ago

@jmichel80 and @annamherz, if cinnebar cannot be integrated yet, can we add the bug fixes to freenrgworkflows and remove the dependency?

Hello! I have the bug fixes as a PR, so if that's okay I'll go ahead and merge it? I'll also have a look at all the analysis stuff to see which is best to use and do any changes on

I will be doing all work on this branch: 04_fep_integration

this branch.

ppxasjsm commented 2 years ago

Ah sorry, just saw the freenrgyworkflows PR, somehow must have been distracted when that happened. I have now merged it.

I have merged the 04_fep_integration already. I've not made any changes yet but just added as is from ccpbiosim repo. I think if we can agree on the style guide, then we can start making swift changes.

annamherz commented 1 year ago

Hello, I have a question re some adjustments for the FEP tutorial. The tutorial currently functions with the feature-workshop branch of BSS. This has changes from the feature-analysis in-cooperated. I think these changes are not yet in devel. I was wondering with which version of BSS the tutorials are intended to work with?

lohedges commented 1 year ago

This is a good question. I was hoping that we could use devel once I've merged the Exscientia ABFE PR, with (some) of the FEP part using the Sandpit, rather than the core code. I'm assuming that you have edits on your feature-analysis branch that aren't in the Exscientia devel, which makes thing tricky.

(The PR is actually very difficult, since they've merged in an early version of your branch, William's work, and some of FInlay's. I am essentially reviewing three separate things, which partially depend on each other, and were based off different versions of our devel.)

annamherz commented 1 year ago

I'm assuming that you have edits on your feature-analysis branch that aren't in the Exscientia devel, which makes thing tricky.

I think the edits on the analysis branch are mainly in _relative.py for things relating to the analysis function, so there aren't too many hopefully, but yes I think they're not in the Exscientia devel.

I was also wondering on whether I should include AMBER as an option for the RBFE? These changes are all in the feature-amber-fep branch, which I've generally tried to keep up-to-date with the devel branch, but I there are quite a few things in it and stuff I might have overlooked when making changes, so I don't know how feasible it is to merge this into devel in time?

since they've merged in an early version of your branch

I'm also not sure how much the current version of it deviates from the one that was initially merged in.

fjclark commented 1 year ago

I forgot to mention that as well as the Exscientia ABFE PR and my PR to Exscientia, the ABFE tutorials require this branch of Sire, which introduces Boresch restraints (pull request for this here). @chryswoods @lohedges, is it likely that this will be merged into devel soon, or will the tutorial have to be modified? Thanks.

annamherz commented 1 year ago
  • Replacement of freenrgyworkflows with arsenic (or whatever the latest name now is) will this happen? Has this happened?

I've added cinnabar analysis to the end of the analysis tutorial, but have also kept in the freenergworkflows for now as well.

  • Do we need a copy of the source of freenrgyworkflows in the tutorial? This seems very dodgy from a coding perspective and can lead to diverging repos.

The freenrgyworkflows it uses is also the one that is in the fep_archiv.

We were hoping to have removed the freenrgworkflows dependency already but I haven't heard much about progress from OpenFE on that end.

I think maybe it might be best to replace it entirely only in a bit.