-
Notifications
You must be signed in to change notification settings - Fork 404
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
New metric module to improve flexibility and intuitiveness #475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome start! Thank you for making this PR! I think it's good step towards cleaning up the messy calculations that happen within the log manager. Aside from the minor comments that I have made, the major question I have is about the way we/users interact withAttackMetric
.
Currently, for each list of attack results, we need to instantiate a fresh AttackMetric
object. Within each __init__
we call calculate
that calculates the desirable metrics and then stores them as instance attributes (e.g. self.total_attacks
). Then, we call various custom instance methods (e.g. successful_attacks_num
, avg_num_queries_num
) to retrieve the attributes.
I see two potential issues with this approach:
(1) The need to instantiate a different AttackMetric
object for each different list of results.
Why do need to create a new AttackMetric
object for any new list of attack results? Is AttackMetric
's role to store the actual metric values for any arbitrary list of attack results, or is it to calculate the metric for a given list of results. If it is the latter, then I think there is no need to create a new instance. Instead, we can create one instance and then reuse it by passing a list of results to a method. This is similar to how our GoalFunction
can be instantiated once and its get_results
method can be used to produce GoalFunctionResults
for list of AttackedText
.
Side note: this is an interesting video that I found that is somewhat related to this. https://www.youtube.com/watch?v=o9pEzgHorH0
(2) Lack of common patterns between different AttackMetric
classes.
We currently have three sub-classes of AttackMetric
that share no common pattern. If I want to calculate the metric, I have to figure out which specific method I have to call (e.g. successful_attacks_num
, avg_num_queries_num
) to get the actual values. This will inevitably become more confusing as we add more metric classes.
Potential Solution (this is only a suggestion, so please feel free to come up with your own solution): Have calculate
method of AttackMetric
take in a list of AttackResult
as an argument and make it the main method that people can use to calculate their metrics. If there are multiple metrics we want to return, we can simply return a dictionary of the values with appropriate keys.
"""A metric for evaluating Adversarial Attack candidates.""" | ||
|
||
@staticmethod | ||
@abstractmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think @staticmethod
here is necessary since it's not really a static method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchit97 please update regarding the comment..
|
||
@staticmethod | ||
@abstractmethod | ||
def calculate(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here. I don't think @staticmethod
should be here (unless you intend to make it a static method). But in other sub-classes you define it as a regular instance method, so I'm guessing this should also be a regular method. Also in that case, you should have self
as the first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchit97 please update regarding the comment..
@@ -0,0 +1,9 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between attack_metrics
module and metrics
module?
"""Calculates all metrics related to number of queries in an attack | ||
|
||
Args: | ||
results (:obj::`list`:class:`~textattack.goal_function_results.GoalFunctionResult`): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but can you check if this works a valid docstring for sphinx? I recall that references don't get nested. Ideally, we want something like list[GoalFunctionResult]
.
You can check by running sphinx-autobuild docs docs/_build/html --port 8765 --host 0.0.0.0
and then connecting to the host+port in your browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchit97 please update regarding the comment..
"""Calculates all metrics related to number of succesful, failed and skipped results in an attack | ||
|
||
Args: | ||
results (:obj::`list`:class:`~textattack.goal_function_results.GoalFunctionResult`): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar thing here. Can you check if docstring works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchit97 please update regarding the comment..
from textattack.attack_results import AttackResult | ||
|
||
|
||
class AttackMetric(AttackResult, ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason why AttackMetric
is a sub-class of AttackResult
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchit97 I have the same concerns as @jinyongyoo above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is an oversight. Fixed.
My major objective to make different |
New metric module to improve flexibility and intuitiveness - moved from #475
What does this PR do?
Summary
This PR adds a new metric module to Textattack, which provides an easier way to add new metrics or use existing metrics on adv. examples. Currently metrics are calculated in the
attack_log_manager.py
file, which is very unintuitive and non-flexible. Slides detailing this change are at: https://docs.google.com/presentation/d/1O77447GGWQF-xDnIdTr8d2MMPsQ9o9CG3pZZoo2bPrs/edit?usp=sharingAdditions
metrics
module astextattack.attack_metrics
.Changes
attack_metrics
is a new module, removing metrics fromattack_log_manager.py
Deletions
Checklist
.rst
file inTextAttack/docs/apidoc
.'