Closed henrykenlay closed 2 years ago
Hi Henry,
Thanks for your valuable suggestions!
For the following comments, it would be really great if you can work on them. If you feel the workload is heavy, you can choose one or two to work and I will work on others.
- Giving BaseAttack.attack an argument list and agree on what input types the methods will be able to accept
- Use Errors instead of assert statements, for example in the PGDAttack.init method
- Replace BaseAttack.save_adj and BaseAttack.save_features with methods that return these variables, rather than save them.
- Add an epochs parameter to PGDAttack.attack
- Including a **kwargs keyword into attack methods that don't use all arguments. This would allow one to change the attack method with very little effort.
For this one,
Using a docstring style such as the numpy/scipy or Google style which gives argument and return descriptions and types
it is also our next step so we will start to work on it recently.
Thank you again for your help!
Hi Wei,
I should be able to manage these tasks, but I'll raise an issue for each so we can track progress and share the workload if needed. Of the five points you've listed above, I will do bullet point 4 as a pull request later because it's just a few lines. The first and fifth bullet point I'll raise together as an issue since its a general refactor to make the classes closer to the base class. The 3rd bullet shouldn't be too much work once the classes are more consistent. Then we can work on the 2nd which may take more work.
Look forward to collaborating!
Great. Thank you : )
I've been using this library to generate global adversarial attacks on graphs using DICE and PGD. I have some suggestions which I'd be happy to implement and put a pull request in for, but I wanted to raise it for your feedback first and check to see if this is something you would be interested in me doing.
Here's a code snippet where I do a DICE and PGD attack I will use to highlight the motivation for my suggestions.
The
DICE
andPGDAttack
classes both subclassBaseAttack
but have slightly different ways to use theattack
method.DICE
the original adjacency is given by keywordadj
but inPGDAttack
it's given byori_adj
DICE
a sparse scipy array is a valid input type but inPGDAttack
it is not.modified_adj
is accessed after the attack is different for both classes.I think the following changes could benefit the
deeprobust/graph/global_attack
APIBaseAttack.attack
an argument list and agree on what input types the methods will be able to accept**kwargs
keyword intoattack
methods that don't use all arguments. This would allow one to change the attack method with very little effort.PGDAttack.__init__
methodBaseAttack.save_adj
andBaseAttack.save_features
with methods that return these variables, rather than save them.epochs
parameter toPGDAttack.attack
Let me know your feedback on these suggestions, I can make them into separate issues which will help us track progress. If you agree with some of these changes I'll be happy to get started on them.