Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing askcos #69

Merged
merged 32 commits into from
May 27, 2024
Merged

Fixing askcos #69

merged 32 commits into from
May 27, 2024

Conversation

kabu00002
Copy link
Collaborator

@kabu00002 kabu00002 commented May 13, 2024

Description

Instead of using the outdated (and not working) Askcos API, we now use Askcos Core V2.

Todos

  • Adapt the implementation
  • Rerun notebook 1_3 and 2_1
  • Update README

Questions

Status

  • Ready to go

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@kabu00002 kabu00002 requested a review from PaulaKramer May 13, 2024 11:41
Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:26Z
----------------------------------------------------------------

The ROMol is not displayed in dataframe


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:27Z
----------------------------------------------------------------

ROMol not displayed here


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:27Z
----------------------------------------------------------------

Same here (ROMol)


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:28Z
----------------------------------------------------------------

We've discussed this but for completeness: GA pocket missing in plot :)


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:30Z
----------------------------------------------------------------

Remove DataWarrior here


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:30Z
----------------------------------------------------------------

Paths have changed - I think it was renamed to data/filters/Enamine


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:31Z
----------------------------------------------------------------

GA missing (I assume it's the same function as above)


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:33Z
----------------------------------------------------------------

Spelling: succesfully -> successfully, installtion -> installation


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:33Z
----------------------------------------------------------------

spelling: installion -> installation


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:35Z
----------------------------------------------------------------

spelling last word (installation)


Copy link

review-notebook-app bot commented May 13, 2024

View / edit / reply to this conversation on ReviewNB

PaulaKramer commented on 2024-05-13T14:46:35Z
----------------------------------------------------------------

Plots missing GA

I'm also a little confused that the B2 pocket is completely empty now... This means that they removed some retrosynthetic pathways in ASKCOS? Because at least the fragments that were found in the earlier customkinfraglib version should technically still be found?


kabu00002 commented on 2024-05-27T07:16:35Z
----------------------------------------------------------------

Not necessarily since we fixed the bug in kinfraglib which might disallow the combination of a B2 fragment with another one, where a retro route was found. Further, the methodology of askcos was changed (?)

Copy link
Collaborator

@PaulaKramer PaulaKramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! I have reviewed the notebooks and added small changes to the readme. Can you add another ASKCOS installation hint in the readme for the data (either in data/filters/ or in data/filters/retro) - I don't really have a preference here :)

Copy link
Collaborator Author

Not necessarily since we fixed the bug in kinfraglib which might disallow the combination of a B2 fragment with another one, where a retro route was found. Further, the methodology of askcos was changed (?)


View entire conversation on ReviewNB

@kabu00002
Copy link
Collaborator Author

Thanks for the changes! I have reviewed the notebooks and added small changes to the readme. Can you add another ASKCOS installation hint in the readme for the data (either in data/filters/ or in data/filters/retro) - I don't really have a preference here :)

Done :)

@kabu00002 kabu00002 merged commit ad138e8 into dev May 27, 2024
0 of 2 checks passed
@kabu00002 kabu00002 deleted the fixing_askcos branch May 27, 2024 07:26
@kabu00002 kabu00002 restored the fixing_askcos branch May 27, 2024 07:29
This was referenced May 27, 2024
@kabu00002 kabu00002 deleted the fixing_askcos branch July 8, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants