volkamerlab / teachopencadd

TeachOpenCADD: a teaching platform for computer-aided drug design (CADD) using open source packages and data
https://projects.volkamerlab.org/teachopencadd
Creative Commons Attribution 4.0 International
713 stars 197 forks source link

T007/22: Switch to fingerpring generator #348

Closed hamzaibrahim21 closed 1 year ago

hamzaibrahim21 commented 1 year ago

Description

Switching fingerprint generators in all talktorials according to #330.

TODO

Status

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

dominiquesydow commented 1 year ago

Hi @hamzaibrahim21,

Thanks a lot for taking on this task 🎉
I have two quick questions before looking at this PR in more detail:

I am adding @AndreaVolkamer for visibility.

hamzaibrahim21 commented 1 year ago

Hi @dominiquesydow

dominiquesydow commented 1 year ago

Hi @hamzaibrahim21,

Thanks a lot for the additional context! :)

All talktorials are checked and the DL-edition as well, so I made an extra box for them, although they have no changes.

I updated the PR description accordingly, thanks!

I repeated Greg's work, as it'd be easier to push all changes to one branch, that is branched from dev branch (If I'm not mistaken).

Repeating someone else's work is I think not ideal, as they have put in work to do this (and should get the credit on GitHub in terms of commits) and it costs you time as well. Did you redo it 1-to-1? If so, we could also merge Greg's branch into this PR here, just to make sure his contribution will be visible on GitHub.

hamzaibrahim21 commented 1 year ago

I thought that would easier for controling/merging branches later and my branch would be merged easily without problems with his branch as I mentioned it in description. sorry for such misunderstanding!! Please let me know if I should reset talktorial 04 and thank you for taking care of this PR :slightly_smiling_face:

dominiquesydow commented 1 year ago

Hi @hamzaibrahim21, no worries at all! My apologies for adding complexity to this PR but I do prefer resetting T004 (could you do that?) and merging Greg's PR into this one (I am going to do that). I think you are right that we might run into conflict issues but I will give solving them a try so that we can include Greg's commit history included. Fingers crossed :)

hamzaibrahim21 commented 1 year ago

@dominiquesydow Great! now T04 should be reset, if anything I should do or take care of, please let me know.

mbackenkoehler commented 1 year ago

@hamzaibrahim21 FYI, you can just run black -l 99 talktorial.ipynb to automatically format the notebook correctly.