Custom pipeline PR - Githubissues

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:42Z ----------------------------------------------------------------

Check if you want to use capitalized words or not!

In a title: Custom Filter Pipeline

In normal text: custom filter pipeline

Also since this is one one the main notebooks: Start with some more information text

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:43Z ----------------------------------------------------------------

-> Filtering pipeline to modify the fragment library:

-> Or: Filtering pipeline to be applied to the fragment library:

again: in English most words are not capitalized!

-> Buyable building block filter

-> Pairwise retrosynthesizability

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:43Z ----------------------------------------------------------------

same here, check which words really nee to be capitalized

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:44Z ----------------------------------------------------------------

Rule of Three (Ro3) - I think this was not defined yet

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:45Z ----------------------------------------------------------------

Just a question, but setting the value to False, would have the same effect as not applying the filter at all, right?

sonjaleo commented on 2021-11-24T10:35:58Z ----------------------------------------------------------------

yes

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:45Z ----------------------------------------------------------------

"unwanted_substructures.csv" file (no '-')
capitalize SMARTS
"unwanted_substructures.csv" (second time also with "")

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:46Z ----------------------------------------------------------------

In the explanation you write "unwanted_substructures.csv", just wondering for which file it is actually looking?

Wouldn't it make more sense to provide also the name of the file in "substurcture_file" instead of just the folder?

sonjaleo commented on 2021-11-24T10:42:43Z ----------------------------------------------------------------

If I provide the file name I would need to change the function and therefore also apply the change in the first notebook.

The other possibility would be to change this parameter to "substructure_file_path" ?

AndreaVolkamer commented on 2021-11-25T11:03:37Z ----------------------------------------------------------------

I think providing the file name would be the cleaner version, but for now changing the parameter as you suggested would work.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:47Z ----------------------------------------------------------------

why is the default set to 6, id there are only five rules (as listed above)?
since one can choose the signs > and <, depending on what one chooses it would not be min but max fulfilled, does it make sense to have the possibility to search for max_fulfilled at all?

sonjaleo commented on 2021-11-24T10:47:19Z ----------------------------------------------------------------

There are six rules. I somehow missed the octanol-water-partition coefficient. Added it now.

I do not really understand the second point?

AndreaVolkamer commented on 2021-11-25T11:07:38Z ----------------------------------------------------------------

If one would choose the cutoff_crit "<" for example, than less than 6 (default) parameters would need to be fulfilled

so a) than six is the upper bound, so the name 'min_fulfilled' might be missleading, because in that case it is max parameters that need to be fulfilled
and b) I think the filter does not make sense for "<" or "<=", because that means that we keep molecules that are not lead like, or?

_sonjaleo commented on 2021-11-25T11:34:08Z_ ----------------------------------------------------------------

So, should I only allow to take > or >=, or should I remove this parameter completely?

When keeping it, I could change it e.g. to num_fulfilled.

_AndreaVolkamer commented on 2021-11-25T11:55:56Z_ ----------------------------------------------------------------

yes, let's restrict it to > and >= here

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:48Z ----------------------------------------------------------------

"For each property, a desirability function is used and with them the estimate is calculated."

-> For each property, a desirability function is used and the QED is calculated as the ......

"To define the cutoff value check: ..."

-> At this point it may not be clear what cut-off value you are talking about? And maybe add if a high or low score are considered better?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:48Z ----------------------------------------------------------------

Mention the plotting functionality above as well?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:49Z ----------------------------------------------------------------

"To manipulate the building block file,"

-> Find more information about the construction and content of the building block file at ...

And the information about what is done with the building blocks is missing?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:50Z ----------------------------------------------------------------

either all comments start with capital letter or none

-> # for creating ...

How about adding a print statement, how many building blocks are contained in the file or so?

sonjaleo commented on 2021-11-24T11:31:08Z ----------------------------------------------------------------

Should I add this only in the pipeline or also in notebook 1_3?

If I should also add it in notebook 1_3 I would need to do this in another PR

AndreaVolkamer commented on 2021-11-25T11:09:17Z ----------------------------------------------------------------

Maybe this and the comments above can be addressed in a separate PR that is not urgent, but a nice to have.

Than you can merge this one already.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:50Z ----------------------------------------------------------------

Mention the plotting part above

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:51Z ----------------------------------------------------------------

"ASKCOS is used to check if for the fragment pairs a retrosynthetic route can be found."

-> mention what is meant by 'fragment pairs' here

ASKCOS webpage for One-step Retrosynthesis

_> ASKCOS webpage for one-step retrosynthesis

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:52Z ----------------------------------------------------------------

Mention the plotting option and what is plotted above

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:53Z ----------------------------------------------------------------

Title 'General parameters' unclear?

If you define general (global?) parameters here, it should maybe be moved to the beginning?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:53Z ----------------------------------------------------------------

It would be good if the next cells, produces an output - like a log file - that summarizes which filters and parameters were applied, so the creation of the fragment library can be well documented.

Also comment on where all the information will be stored? When is the custom fragment library actually created?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:54Z ----------------------------------------------------------------

It might be helpful to show the full dataframe - with all filtering information combined - at the end of the above output?

sonjaleo commented on 2021-11-24T12:17:53Z ----------------------------------------------------------------

which "full" dataframe?

AndreaVolkamer commented on 2021-11-25T11:13:07Z ----------------------------------------------------------------

I meant sth like the custom filter results, that you also read in later would give a nice overview here already

sonjaleo commented on 2021-11-25T11:41:13Z ----------------------------------------------------------------

Should I just move it to the top of this point?

AndreaVolkamer commented on 2021-11-25T11:56:20Z ----------------------------------------------------------------

yes that works

Addition to your addition: How about printing it here, and keep the reload were it currently is?

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:55Z ----------------------------------------------------------------

What do we need these single dataframes for? It is usually easier to keep track, if all lives in one large dataframe, no?

Or maybe not, tbd.

In any case, add some description what is done here

sonjaleo commented on 2021-11-24T12:42:54Z ----------------------------------------------------------------

Those are all the dataframes that are also created in the other notebooks. If I change this here, I would have to change everything in all the other notebooks as well.

It is more "additional" information to look at if interested, the mol_df and diff_df are also not possible to be included as there is more than one information for some fragments.

I will add a description to all of these files and what is inside.

AndreaVolkamer commented on 2021-11-25T11:10:26Z ----------------------------------------------------------------

OK thanks. A description should be enough for now.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

AndreaVolkamer commented on 2021-11-23T14:01:55Z ----------------------------------------------------------------

Was it mentioned above where the custom fragment library is stored?

sonjaleo commented on 2021-11-24T12:45:12Z ----------------------------------------------------------------

It is defined in the global parameters. I will also add a text, describing it because we want to store the newly generated fragment libraries in the timestamp folders

sonjaleo commented 2 years ago

yes

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

If I provide the file name I would need to change the function and therefore also apply the change in the first notebook.

The other possibility would be to change this parameter to "substructure_file_path" ?

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

There are six rules. I somehow missed the octanol-water-partition coefficient. Added it now.

I do not really understand the second point?

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

Should I add this only in the pipeline or also in notebook 1_3?

If I should also add it in notebook 1_3 I would need to do this in another PR

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

which "full" dataframe?

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

Those are all the dataframes that are also created in the other notebooks. If I change this here, I would have to change everything in all the other notebooks as well.

It is more "additional" information to look at if interested, the mol_df and diff_df are also not possible to be included as there is more than one information for some fragments.

I will add a description to all of these files and what is inside.

View entire conversation on ReviewNB

sonjaleo commented 2 years ago

It is defined in the global parameters. I will also add a text, describing it because we want to store the newly generated fragment libraries in the timestamp folders

View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago

I think providing the file name would be the cleaner version, but for now changing the parameter as you suggested would work.

View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago

If one would choose the cutoff_crit "<" for example, than less than 6 (default) parameters would need to be fulfilled

so a) than six is the upper bound, so the name 'min_fulfilled' might be missleading, because in that case it is max parameters that need to be fulfilled
and b) I think the filter does not make sense for "<" or "<=", because that means that we keep molecules that are not lead like, or?

--- View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago

Maybe this and the comments above can be addressed in a separate PR that is not urgent, but a nice to have.

Than you can merge this one already.

View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago

OK thanks. A description should be enough for now.

View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago

I meant sth like the custom filter results, that you also read in later would give a nice overview here already

View entire conversation on ReviewNB

AndreaVolkamer commented 2 years ago