search

yigbt / deepFPlearn

Link molecular structures of chemicals (in form of topological fingerprints) with multiple targets
Other
8 stars 6 forks source link

Soulios graphs #22

Closed soulios closed 1 year ago

bernt-matthias commented 1 year ago

There seem to be still conflicts: This branch has conflicts that must be resolved

My strategy would be the following:

If needed I can help next week. Should be around Monday and Wednesday.

Have a nice git-free weekend :)

soulios commented 1 year ago

Can you assign it to me and put someone else as a reviewer? so I can resolve some conflicts myself.

bernt-matthias commented 1 year ago

@soulios cool that there are no more conflicts!

I started the workflows. Linter and tests revealed some (hopefully) minor problems :) Could you take care of these? If I find some time I may add some comments earlier.

ping @halirutan maybe you can also add some first comments.

mai00fti commented 1 year ago

@soulios

flake and pytests checks have failed. Please resolve respective issues

bernt-matthias commented 1 year ago

Not sure if I understand why this has been merged yet.

what should I do here in the setup.py where I install my fork of chemprop with "chemprop @ git+https://github.com/soulios/chemprop.git@1d73523e49aa28a90b74edc04aaf45d7e124e338"

The solution^{TM} is to create a proper pull request of your changes upstream!

I also have to note that this is a blocker conda and Galaxy deployment .. besides being incredibly hard to maintain .. I would say impossible (because someone needs to sync this with upstream changes of chemprop).

soulios commented 1 year ago

But if the user installs my version of chemprop at this point, any code will be running. Why should it be always the latest version of chemprop? Mine has a few extra functionalities that the original project does not. i.e tracking the training metrics. I have asked them before if they would like it and said no(in a private chat). So I dont think I can open a pr to them.or shall I and we can decide from there?

bernt-matthias commented 1 year ago

But if the user installs my version of chemprop at this point, any code will be running.

True

Why should it be always the latest version of chemprop?

I'm just saying that it's quite an effort to get updates (like bug fixes and improvements) .. my pessimistic prediction would be that we will be stuck with this version for a long time.

Also, think about the wording in a publication. We can't just write we used chemprop version x.y, but we used a modified version of chemprop and describe in detail what has been changed?

I have asked them before if they would like it and said no(in a private chat). So I don't think I can open a pr to them.or shall I and we can decide from there?

Sure. But it needs to be well prepared. At the moment a few changes seem wrong (e.g. changes in the imports). It's also important to submit PR(s) that are as small/atomic as possible. It's probably a good idea to make all new functionality optional.

We also should think twice if at least some of the changes may be implemented externally (or if we can suggest a change of chemprop such that it can be implemented externally).

bernt-matthias commented 1 year ago

Not sure if I understand why this has been merged yet.

Just to clarify: For some reason (temporary problem at github ...), the automated testing did not run for the last commits prior to the merge. Thus the PR may have appeared successful, but it wasn't (at least tests are failing locally).