Adding class BlobLoader to separate loading from JsonIndexDataset logic #1463

salaxieb · 2023-02-28T16:20:51Z

added class BlobLoader which is responsible for loading blob for given FrameData
moved loading function to separate file load_blob.py

shapovalov · 2023-02-28T20:04:05Z

pytorch3d/implicitron/dataset/json_index_dataset.py

-            R=torch.tensor(entry_viewpoint.R, dtype=torch.float)[None],
-            T=torch.tensor(entry_viewpoint.T, dtype=torch.float)[None],
-        )
+        return self.blob_loader.load(frame_data, entry, point_cloud)


I’d suggest to pass self.seq_annots[entry.sequence_name] here, as we can later add other sequence-level blobs, not just point clouds.

shapovalov

Thanks for submitting it so quickly! Overall looks good.
@bottler – do you want to take a look at the overall idea? For the context, this change will be tested together with SqlDataset refactoring in pixar_replay, hence Ildar is developing here and is going to import to fbcode once everything works.

shapovalov · 2023-02-28T20:05:26Z

pytorch3d/implicitron/dataset/load_blob.py

+
+    path_manager: Any = None
+
+    def __init__(


Please make it a dataclass or a Configurable to avoid boilerplate.

shapovalov · 2023-02-28T23:25:25Z

pytorch3d/implicitron/dataset/load_blob.py

@@ -0,0 +1,545 @@
+import functools


Let’s call the module blob_loader to match the class name.

shapovalov · 2023-02-28T23:30:15Z

pytorch3d/implicitron/dataset/load_blob.py

+        self.box_crop_mask_thr: float = box_crop_mask_thr
+        self.box_crop_context: float = box_crop_context
+
+    def load(


Since this method returns FrameData, it will be unclear if it modifies the object or copies it. Do we need to return it? At least please make sure to document that frame_data is modified in-place.

…-loading

shapovalov · 2023-03-06T14:00:59Z

tests/implicitron/test_blob_loader.py

+        sequence_file = os.path.join(dataset_root, category, "sequence_annotations.jgz")
+        self.image_size = 256
+
+        expand_args_fields(JsonIndexDataset)


This should be redundant with modern pytorch3d.

salaxieb · 2023-03-09T15:42:03Z

I'm not sure about last commit
Fact that it's nice to check all the frames, but same time it makes test longer
I can choose a sweet spot of checking every 200 frames for example. Give me your thoughts..

shapovalov

Thanks for adding the verbose test!
I think it should be enough to test on 1 frame only – it is unlikely we can uncover any problem by loading multiple images of the same type.

Also, in a spirit of a unit test, you can make it more lightweight. Currently, the setup includes creating a JsonIndexDataset, which is a lot of stuff to do. If creating a dataset crashes, we don’t even get to testing a loader.
As an option, you can store all relative paths for one frame and test loading functions on them, and for other tests, create FrameAnnotation / FrameData objects with relevant fields set.

shapovalov · 2023-03-09T17:41:06Z

tests/implicitron/test_blob_loader.py

-    def test_load_mask(self):
-        path = os.path.join(self.dataset_root, self.entry.mask.path)
+    def _load_mask_test(self, entry):
+        path = os.path.join(self.dataset_root, entry.mask.path)


Please use methods like self.assertEqual as they give better diagnostics.

bottler

This is a good change: splitting the logic makes the dataset object much easier to understand, even before we start looking at new implementations.

bottler · 2023-03-13T10:27:54Z

pytorch3d/implicitron/dataset/json_index_dataset.py

@@ -65,7 +63,7 @@ class JsonIndexDataset(DatasetBase, ReplaceableBase):
    A dataset with annotations in json files like the Common Objects in 3D
    (CO3D) dataset.

-    Args:
+    Metadata-related args::


This distinction between the two types of members doesn't matter to the user of this class; it's an implementation detail. We could leave as a big list in this comment, or we could split into groups like "Options affecting WHICH frames are loaded"/"Options affecting what data is loaded for each frame"/"Misc options".

pytorch3d/implicitron/dataset/blob_loader.py

bottler · 2023-03-13T10:33:46Z

pytorch3d/implicitron/dataset/blob_loader.py

+    box_crop_context: float = 0.3
+    path_manager: Any = None
+
+    def load(


We could call this function load_ to reinforce the inplaceness.

Would it hint to modifying BlobLoader state rather than FrameData state though?

Good point. Not sure.

Co-authored-by: Jeremy Reizenstein <[email protected]>

…x to FrameData Summary: extracted blob loader added documentation for blob_loader did some refactoring on fields for detailed steps and discussions see: #1463 fairinternal/pixar_replay#160 Reviewed By: bottler Differential Revision: D44061728 fbshipit-source-id: eefb21e9679003045d73729f96e6a93a1d4d2d51

Ildar Salakhiev added 4 commits February 28, 2023 15:23

created class BlobLoader and moved all related function to sep file

aa34aa0

added type hints and deleted chore pyre-ignore

f745dfc

linter

c3c5110

linter

9b431bd

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 28, 2023

salaxieb and others added 3 commits February 28, 2023 16:21

Merge branch 'main' into main

c74261d

deleted chore pyre-ignore

627e60f

Merge branch 'main' of github.com:salaxieb/pytorch3d

d0a2d4d

shapovalov reviewed Feb 28, 2023

View reviewed changes

shapovalov requested review from bottler and davnov134 February 28, 2023 20:17

shapovalov reviewed Feb 28, 2023

View reviewed changes

Ildar Salakhiev added 8 commits March 1, 2023 09:49

renamed load_blob to blob_loader

0aa27a6

sending to BlobLoader whore seq_annotation

53823cf

made blob_loader dataclass to avoid boilerplate

d6f13eb

documented, that FrameData modification done inplace

86e64f7

spliited JsonIndexDataset args to 2 gorups: Matadata-related and Blob…

2f17049

…-loading

code refactoring to delete chore pyre-ignore

527ec09

deleted chore function

24b731b

BloabLoader tests boilerplate

f484a12

shapovalov reviewed Mar 6, 2023

View reviewed changes

Ildar Salakhiev added 9 commits March 7, 2023 13:11

tests WIP (not tested)

b8674ea

tests typos and errors WIP

faeffcf

tests typos and errors WIP

bc24e29

solved error and typos for test_bbox

e9c5969

updating test_blob_loader WIP

44cfcfb

blob loader tests ready for review

11def0a

typo

bc52382

typo

0149377

linter

3bcbd01

all entry tests run thru all frames

269cffa

shapovalov reviewed Mar 9, 2023

View reviewed changes

Ildar Salakhiev added 9 commits March 10, 2023 09:38

assert .. == .. to self.assertEqual(.., ..)

f930d71

testing only on 1 frame

dc7a702

instead of loading whole dataset, loading only single frame annots

fcd8d8b

added default values to BlobLoader to ease initialisation

c3bd722

mackink tests on single loaded frame

cb34c01

made _resize_image separate function (will ease use in pixar replay)

04b7d15

type in function arguments

76f45aa

moved tests for _resize_image to test_bbox

e5d3a2b

np array instead of tensor to resize_image

1ba1a3a

bottler reviewed Mar 13, 2023

View reviewed changes

Ildar Salakhiev and others added 3 commits March 13, 2023 10:59

setting up default scale value to correct one

cd9aa5c

renamed funciton to load_ to make more obvious inplace modification

ce9fd40

Update pytorch3d/implicitron/dataset/blob_loader.py

46d39ed

Co-authored-by: Jeremy Reizenstein <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding class BlobLoader to separate loading from JsonIndexDataset logic #1463

Adding class BlobLoader to separate loading from JsonIndexDataset logic #1463

salaxieb commented Feb 28, 2023

shapovalov Feb 28, 2023

shapovalov left a comment

shapovalov Feb 28, 2023

shapovalov Feb 28, 2023

shapovalov Feb 28, 2023

shapovalov Mar 6, 2023

salaxieb commented Mar 9, 2023

shapovalov left a comment

shapovalov Mar 9, 2023

bottler left a comment

bottler Mar 13, 2023

bottler Mar 13, 2023

shapovalov Mar 13, 2023

bottler Mar 13, 2023

Adding class BlobLoader to separate loading from JsonIndexDataset logic #1463

Are you sure you want to change the base?

Adding class BlobLoader to separate loading from JsonIndexDataset logic #1463

Conversation

salaxieb commented Feb 28, 2023

Choose a reason for hiding this comment

shapovalov 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

salaxieb commented Mar 9, 2023

shapovalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bottler 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