Refactor feature vector generation #394

hdefazio · 2024-04-15T18:08:18Z

time_from_name() now requires the topic of the data it is being provided because the medical and cooking dataset images have different filename structures
- this also required task specific versions of time_from_name() in their respective load_data files
Added the ability to draw keypoints if they are present in visualize_kwcoco_by_label()
Wrote hacky function activity_label_fixes() to map the BBN ground truth acitivty labels to our activity labels

MAIN POINT OF MR:

Refactored compute_feats() in train_actiivty_classifier.py and obj_det2d_set_to_feature() and obj_det2d_set_to_feature_by_method() in utils.py so the joint distances would be added if the respective flags were turned on in utils.py and not always added in train_activity_classifier.py
- this also resulted in refactoring the feature vector order to remove redundancies in the data and loops
Added plot_feature_vec() to plot the various object/hand/joint points based on the feature vector and the kwcoco hand bounding boxes as a debug and visualization tool
Added feature_version_to_options() so we can access the corresponding feature vector flags in new spots - and thus updated obj_det2d_set_to_feature() to use this function

squash me

cameron-a-johnson · 2024-05-01T15:22:25Z

angel_system/data/medical/load_bbn_data.py

+    Extract the float timestamp from the filename.
+
+    :param fname: Filename of an image in the format
+        frame_<frame number>_<seconds>_<nanoseconds>.<extension>


Is there somewhere else that it would be useful to document this filename requirement?

Another idea: wherever such a filename requirement is assumed, we could add an "assert" checking for the right number of underscores

^From Hannah:

the id used to be parsed like this:

RE_FILENAME_TIME = re.compile( r"frame_(?P<frame>\d+)(?P<ts>\d+(?:|.)\d+).(?P<ext>\w+)" )

So then you'd get a slightly-opaque error in this code:

fname = os.path.basename(fname) match = RE_FILENAME_TIME.match(fname) time = match.group("ts")

Rough proposal: edit the function here and always use it to acquire any arguments from the assumed filename above. Add some assertions (correct number of underscores, etc.) which try to enforce the above naming convention. If those assertions fail, the failure message should contain the fname convention that's documented in-line above.

@Purg , please tell me if you have better ideas or suggestions for the check I'm proposing here!

Late to the party. I would probably recommend updating the regex to have more specific captures for the expected pattern instead of capturing a variable pattern and then performing secondary parsing. Unless you're implying with the regex that the _<nanoseconds> portion is optional and sometimes not there. If the latter is the case, I would still recommend being explicit and having two regexs, one with and one without nanoseconds. Try matching against nanoseconds, if fail try parse against only seconds, if fail then raise an exception.

RE_FILENAME_TIME_SEC = re.compile( r"frame_(?P<frame>\d+)_(?P<ts_sec>\d+)\.(?P<ext>\w+)" ) RE_FILENAME_TIME_NANO = re.compile( r"frame_(?P<frame>\d+)_(?P<ts_sec>\d+)_(?P<ts_nano>\d+)\.(?P<ext>\w+)" )

hdefazio · 2024-05-01T15:28:04Z

angel_system/data/common/load_data.py

@@ -29,6 +29,14 @@ def sanitize_str(str_: str):
    """
    return str_.lower().strip(" .")

+def time_from_name(fname, topic="cooking"):


add some assertion that the fname matches the RE_FILENAME_TIME parsing that we expect - if it doesn't, print RE_FILENAME_TIME

hdefazio · 2024-05-01T15:31:44Z

angel_system/data/common/load_data.py

+    if topic == "medical":
+        from angel_system.data.medical.load_bbn_data import time_from_name as tfn
+    elif topic == "cooking":
+        from angel_system.data.cooking.load_kitware_data import time_from_name as tfn


Is this the right way to handle multiple versions of this function? This was attempting to allow other functions to just import from angel_system.data.common.load_data import time_from_name without needing to recreate this if statement each time

This is OK, but it is generally preferred to not perform imports at runtime if you can avoid it (unexpected runtime exception hazard). An alternative to this is to create a global dict (or equivalent structure) that has these keys as its keys, and its values being the function reference, e.g.

... import angel_system.data.medical.load_bbn_data import angel_system.data.cooking.load_kitware_data ... TOPIC_TIME_FN_MAP = { "medical": angel_system.data.medical.load_bbn_data.time_from_name, "cooking": angel_system.data.cooking.load_kitware_data.time_from_name, } def time_from_name(fname, topic="cooking"): topic = topic.lower() if topic not in TOPIC_TIME_FN_MAP: raise ValueError(f"Unknown topic {topic}: Unknown filename time function.") return TOPIC_TIME_FN_MAP[topic](fname)

angel_system/data/medical/load_bbn_data.py

cameron-a-johnson · 2024-05-01T15:54:44Z

angel_system/data/medical/load_bbn_data.py

-        label = "mark-time"
-        label_id = 8
-
+def activity_label_fixes(task, activity_label, target):


This is hacky - the way BBN gave us activity GT, we parse it like this in angel_system/data/medical/load_bbn_data.py:

data = line.split("\t") # Find frame filenames start_frame = int(data[0]) end_frame = int(data[1]) # Determine activity activity_str = data[2].strip().split(" ") hand = activity_str[0] activity = activity_str[1] target = activity_str[2] if len(activity_str) > 2 else None

Example output:

212 1128 hands apply_pressure_to casualty_wound

(Paraphrased)
"start frame"
"end frame"
"which hand is doing the activity" ("both" = "hands")
"activity" (generic, could apply to R18 or M2)
"target"

Should we add some text like this to the docstring for this function?

Purg

I'm not sure I am able to grasp what is going on at the larger picture view, but the smaller pieces that I could understand look good to me. See relatively minor comments.

I'll assume various leading/trailing whitespace things will be handled via a code formatting pass with black.

Purg · 2024-05-06T18:41:01Z

angel_system/activity_classification/train_activity_classifier.py

-        f"{json.dumps([o['name'] for o in dset.categories().objs])}"
-    )
-    act_id_to_str = {dset.cats[i]["id"]: dset.cats[i]["name"] for i in dset.cats}
+    print(f"Object label mapping:\n\t", obj_label_to_ind)


Nitpick: This is not an f-string anymore so the leading f can be removed.

Purg · 2024-05-06T18:41:47Z

angel_system/activity_classification/train_activity_classifier.py

-
+    :param feat_version:
+        Version of the feature conversion approach.
+    :param top_k_objects: Number top confidence objects to use per label, defaults to 1


Suggested change

:param top_k_objects: Number top confidence objects to use per label, defaults to 1

:param top_k_objects: Number of top confidence objects to use per label, defaults to 1

Purg · 2024-05-06T18:44:02Z

angel_system/activity_classification/train_activity_classifier.py

-    object_inds = list(
-        set(list(label_to_ind.values())) - set(hands_inds) - set(non_object_inds)
-    )
+    zero_joint_offset = [0 for i in range(22)]


Are we able to get this 22 size from anywhere via introspection, or is the only not-difficult way to get this in here via hard-coding?

Purg · 2024-05-06T18:47:48Z

angel_system/activity_classification/train_activity_classifier.py

        X.append(feature_vec.ravel())
-
+         


trailing whitespace.

Purg · 2024-05-06T18:50:06Z

angel_system/activity_classification/utils.py

+#########################
+default_dist = (0, 0)  # (1280 * 2, 720 * 2)
+default_center_dist = (0, 0)  # (1280, 720)
+default_bbox = [0, 0, 0, 0]  # [0, 0, 1280, 720]


Ideally, non-mutable types should be used as defaults (and for function argument defaults that I see later), e.g. tuple. The use of list type variables here means they are subject to modification during runtime which can lead to hard-to-debug logical errors.

Purg · 2024-05-06T19:43:00Z

angel_system/activity_classification/utils.py

    if use_center_dist:
-        image_center = kwimage.Boxes([default_bbox], "xywh").center
+        image_center = kwimage.Boxes([0, 0, 1280, 720], "xywh").center # Hard coded image size


I would be a little more comfortable with default values/tuples/etc. being defaulted kwargs to the function (actually modifiable by future/advanced code), but if this works for now, it works. Either way, I would highly recommend documenting hard-coded values and assumptions in the function's doc-string.

Purg · 2024-05-06T19:47:38Z

angel_system/data/common/kwcoco_utils.py

@@ -49,7 +49,7 @@ def load_kwcoco(dset):
    return dset


-def add_activity_gt_to_kwcoco(task, dset):
+def add_activity_gt_to_kwcoco(topic, task, dset, activity_config_fn):


I think I latently understand that "topic" here refers to things like "Medical", "Cooking", etc. It would be good to define jargon like this here so that future readers know what should be going here.

Purg · 2024-05-06T19:54:49Z

angel_system/data/common/load_data.py

+    if topic == "medical":
+        from angel_system.data.medical.load_bbn_data import time_from_name as tfn
+    elif topic == "cooking":
+        from angel_system.data.cooking.load_kitware_data import time_from_name as tfn


This is OK, but it is generally preferred to not perform imports at runtime if you can avoid it (unexpected runtime exception hazard). An alternative to this is to create a global dict (or equivalent structure) that has these keys as its keys, and its values being the function reference, e.g.

... import angel_system.data.medical.load_bbn_data import angel_system.data.cooking.load_kitware_data ... TOPIC_TIME_FN_MAP = { "medical": angel_system.data.medical.load_bbn_data.time_from_name, "cooking": angel_system.data.cooking.load_kitware_data.time_from_name, } def time_from_name(fname, topic="cooking"): topic = topic.lower() if topic not in TOPIC_TIME_FN_MAP: raise ValueError(f"Unknown topic {topic}: Unknown filename time function.") return TOPIC_TIME_FN_MAP[topic](fname)

Purg · 2024-05-06T19:57:39Z

angel_system/data/common/load_data.py

+    if topic == "medical":
+        from angel_system.data.medical.load_bbn_data import time_from_name
+    elif topic == "cooking": 
+        from angel_system.data.cooking.load_kitware_data import time_from_name


The above time_from_name could be used here instead of duplicating its functionality.

Purg · 2024-05-06T19:59:54Z

angel_system/data/medical/load_bbn_data.py

+    Extract the float timestamp from the filename.
+
+    :param fname: Filename of an image in the format
+        frame_<frame number>_<seconds>_<nanoseconds>.<extension>


This comment doesn't match the regex defined above: The regex has a - (dash) after the frame number instead of an underscore.

hdefazio marked this pull request as draft April 15, 2024 18:08

Hannah DeFazio added 8 commits April 25, 2024 12:14

Switch function to obj_det2d_set_to_feature

2abafa7

Remove descriptors and obj-obj/obj-hand inputs

106ea04

Only use one item per class

27502fc

Add print statement

0fe6e75

wip: move joint distance calculations

a527e4e

wip

0b9fd6e

Add r18 actions mappings; bug fixes to activity text to csv function

1092c52

squash me

Bug fixes; move time_from_name to be topic specific

f8ecf0a

hdefazio force-pushed the fix/pytest branch from fb0b6f8 to f8ecf0a Compare April 25, 2024 16:17

Hannah DeFazio added 2 commits April 25, 2024 12:33

Add print statement for the hand model classes

12d739c

Remove unused imports

162b77e

hdefazio mentioned this pull request Apr 25, 2024

Refactor data generation PTG-Kitware/TCN_HPL#23

Merged

Hannah DeFazio added 8 commits April 26, 2024 12:27

Draw pose keypoints

c479505

squash me

bbafb4f

Draw the feature vector

bf3d78e

Remove decimal from version

b4e3229

Update time_from_name call

acc8e97

Add time_from_name here so we don't break imports

ed9b11b

Add top k confidence objects to feature vector

73ee5ee

Update documentation for feature version 5

87e4049

hdefazio changed the title ~~Fix pytest failures~~ Refactor feature vector generation May 1, 2024

cameron-a-johnson reviewed May 1, 2024

View reviewed changes

hdefazio commented May 1, 2024

View reviewed changes

angel_system/data/medical/load_bbn_data.py Show resolved Hide resolved

cameron-a-johnson reviewed May 1, 2024

View reviewed changes

Docstrings

2b43b95

Purg reviewed May 6, 2024

View reviewed changes

Grab the correct top_k index when using hand data

c0c9201

Hannah DeFazio and others added 18 commits May 8, 2024 17:12

Reorder the feature vector

ff9fe76

Fix bug where top k objects were not in confidence order

d097d01

Fix indexing bug when top k is greater than 1

4665fb6

Fix variable name

6dfaf5b

wip: Fixing feature vector generation

88ec401

Fixed merge conflicts from stash

6534c5e

Make augmentations in TCN optional

887277c

fix conflict

ed053ea

update tcn submodule

e63024e

add topic argument

9eaab3b

Change default top-k to k=1

93bf549

update the feature vector generation function call

0bc6854

add topic argument to ros node

2020864

add normalization argument

7c0a7f4

change default window size to 25

4d3462d

do not add patient or user to detection label list

27cf294

provisioning TCN v6 for R18 activity classification model

6664229

lint

7c419e5

hdefazio marked this pull request as ready for review May 11, 2024 01:16

hdefazio merged commit cbc2cfd into PTG-Kitware:master May 11, 2024
1 of 3 checks passed

hdefazio deleted the fix/pytest branch May 11, 2024 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor feature vector generation #394

Refactor feature vector generation #394

hdefazio commented Apr 15, 2024 •

edited

Loading

cameron-a-johnson May 1, 2024

cameron-a-johnson May 1, 2024

cameron-a-johnson May 1, 2024

cameron-a-johnson May 1, 2024

Purg May 6, 2024

hdefazio May 1, 2024

hdefazio May 1, 2024

Purg May 6, 2024

cameron-a-johnson May 1, 2024

cameron-a-johnson May 1, 2024

Purg left a comment

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

Purg May 6, 2024

	:param top_k_objects: Number top confidence objects to use per label, defaults to 1
	:param top_k_objects: Number of top confidence objects to use per label, defaults to 1

Refactor feature vector generation #394

Refactor feature vector generation #394

Conversation

hdefazio commented Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Purg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdefazio commented Apr 15, 2024 •

edited

Loading