Skip to content

Conversation

@ahuller3
Copy link

@ahuller3 ahuller3 commented Sep 9, 2021

Broke eval_tracks into smaller functions. Still needs comments and some type annotations.

broke eval_tracks into smaller functions
@ahuller3 ahuller3 closed this Sep 20, 2021
@ahuller3 ahuller3 reopened this Sep 20, 2021


def eval_tracks(
def get_frame_data(
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @ahuller3, how long does get_frame_data() take compared to the mot metrics eval function call?

Copy link
Contributor

@mgilson-argo mgilson-argo left a comment

Choose a reason for hiding this comment

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

I was asked to look at PRs in here and I didn't realize until just now that this is still a draft -- sorry for any spam, but I figured I'd submit the comments I have to save you pain later.

acc_c,
acc_i,
acc_o,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

No return type? Also no types on acc_c, acc_i, acc_o ...

Also, this function has way too many arguments. It seems like we probably needs a refactor.


for ind_frame in range(len(path_track_data)):
if ind_frame % 50 == 0:
logger.info("%d/%d" % (ind_frame, len(path_track_data)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably:

logger.info("%d/%d", ind_frame, len(path_track_data))

if ind_frame % 50 == 0:
logger.info("%d/%d" % (ind_frame, len(path_track_data)))
if not os.path.exists(path_gt):
logger.warning("Missing ", path_gt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a placeholder in this log format string.

def write_summary_to_file(
acc_c,
acc_o,
ID_gt_all: List[str],
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually have to be List or can it be Collection or Iterable or Sequence?

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