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

[Feature] Support ActivityNetMeanAR #84

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

hukkai
Copy link
Collaborator

@hukkai hukkai commented Jan 19, 2023

WIP

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@C1rN09
Copy link
Collaborator

C1rN09 commented Feb 14, 2023

Is this PR ready for review, or still WIP? ^_^

@hukkai
Copy link
Collaborator Author

hukkai commented Feb 21, 2023

@C1rN09 Hi, it is ready

Copy link
Collaborator

@C1rN09 C1rN09 left a comment

Choose a reason for hiding this comment

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

There should be 2 additional modifications:

  1. Add unit tests for this metric. NOTE that 2 basic use cases should be covered: (1) stateless call, i.e. directly use metric's __call__ method to obtain result. (2) statefull call, i.e. call metric.add multiple (>=2) times and then metric.compute
  2. Add api docs in metrics.rst

mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
@hukkai hukkai requested a review from C1rN09 February 28, 2023 02:36
@hukkai
Copy link
Collaborator Author

hukkai commented Feb 28, 2023

@C1rN09 Not sure why the link failed, could you have a look?

@C1rN09
Copy link
Collaborator

C1rN09 commented Feb 28, 2023

@C1rN09 Not sure why the link failed, could you have a look?

This has been fixed in #86
You can rebase to newest master branch

mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
mmeval/metrics/anet_ar.py Outdated Show resolved Hide resolved
@hukkai hukkai requested a review from C1rN09 March 2, 2023 21:38
Copy link
Collaborator

@C1rN09 C1rN09 left a comment

Choose a reason for hiding this comment

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

LGTM. There should be a PR in mmaction2 to check the accuracy of this implementation.

Comment on lines +9 to +10
logger = logging.getLogger(__name__)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since PR #102, logger has became an argument in BaseMetric, so other metrics no longer need to define logger themselves.

Comment on lines +23 to +24
t_iou (np.ndarray): 1-dim array [n] /
2-dim array [n x m] with IoU ratio.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
t_iou (np.ndarray): 1-dim array [n] /
2-dim array [n x m] with IoU ratio.
t_iou (np.ndarray): 1-dim array [n] /
2-dim array [n x m] with IoU ratio.

Comment on lines +27 to +30
if target_segments.ndim != 2:
raise ValueError('Dimension of target segments is incorrect')
if candidate_segments_ndim not in [1, 2]:
raise ValueError('Dimension of candidate segments is incorrect')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if target_segments.ndim != 2:
raise ValueError('Dimension of target segments is incorrect')
if candidate_segments_ndim not in [1, 2]:
raise ValueError('Dimension of candidate segments is incorrect')
if target_segments.ndim != 2:
raise ValueError(f'Dimension of target segments is incorrect. Expected 2, got {target_segments.ndim}')
if candidate_segments_ndim not in [1, 2]:
raise ValueError(f'Dimension of candidate segments is incorrect. Expected 1 or 2, got {target_segments.ndim}')

Comment on lines +116 to +120
if this_video_proposals.ndim != 2:
this_video_proposals = np.expand_dims(this_video_proposals, axis=0)
if this_video_ground_truth.ndim != 2:
this_video_ground_truth = np.expand_dims(
this_video_ground_truth, axis=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When will these 2 if-statements be True? There has been

this_video_proposals = proposals_video_id[:, :2]

and

this_video_ground_truth = ground_truth_video_id[:, :2].astype(np.float32)

before.

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.

3 participants