-
Notifications
You must be signed in to change notification settings - Fork 20
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
Merged procthor code #39
base: main
Are you sure you want to change the base?
Conversation
e897e03
to
4d46c24
Compare
Moved procthor-related modules as submodules of rearrange ones Removed redundant procthor_rearrange_constants Updated requirements and conda environment Doc updates + removed ai2thor_rearrangement prefix for data paths in inv tasks Typo Update README.md Fixed base dirs for ProcTHOR commands Fixed base config vo ithor_mini_valid eval config Update README.md Added TOC entry for ProcTHOR pre-training Doc update + bug fix in data path constant Missing ctx in call to split_data
4d46c24
to
4ef5bb1
Compare
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.
Very nice :)! There are a huge number of new lines so I've only given things a quick review, if there are any particular files/functions/lines you'd like me to look at in detail let me know. I have two main suggested changes:
- There are a few cases where we seem to have copied a bunch of code from one place to another with some minor changes. Can we try to merge these?
- I'm not a big fan of the
git-lfs
dependency (indeed it caused a problem when I tried to grab this branch locally). Can we remove all the git-lfs tracked files and put them into their own, separate, repository? Then we can useprior.load_dataset(...)
to load data from that repository and keep everything in this repo git-only.
.gitattributes
Outdated
data/2022procthor/mini_val_consolidated.pkl.gz filter=lfs diff=lfs merge=lfs -text | ||
data/2022procthor/split_mini_val filter=lfs diff=lfs merge=lfs -text | ||
data/2022procthor/split_mini_val/** filter=lfs diff=lfs merge=lfs -text | ||
data/2022procthor/split_train filter=lfs diff=lfs merge=lfs -text | ||
data/2022procthor/split_train/** filter=lfs diff=lfs merge=lfs -text | ||
data/2022procthor/train_consolidated.pkl.gz filter=lfs diff=lfs merge=lfs -text |
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.
Does this mean we have a dependency on git-lfs
? Was this the issue we were talking about yesterday regarding how the prior
library handles things?
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.
In this case, I was actually not planning on using prior to distribute the datasets, but rather following the current design of directly hosting the data in the repository (with the modification of using git-lfs
to keep future changes in the data from piling up in the history). If I'm right, we just need to clone the repo and all the procthor data is available, as with the iTHOR data.
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.
Gotcha, I'd prefer we didn't introduce git-lfs
as a dependency here as it's yet another thing to download and install (and getting this repository working in a new environment is already quite a lot for people). In the prior
package I do some things behind the scenes so that we download a git-lfs
binary onto the users machine in the background if someone doesn't have it so this is why the git lfs dependency isn't as much of an issue there.
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 see. I guess I wasn't realizing I was able to directly clone the repository precisely because I had already installed git-lfs
when I pushed the datasets. I'm not super confident about how to properly use prior
to distribute the data, but I guess the example for procthor-10k will do.
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.
In order to keep things more consistent, wouldn't it make more sense to just keep everything in this repo, i.e. without git-lfs given its downsides? We will still have to explain how to install the data in a reachable path (e.g. via additional instructions in README) if we use prior
.
Let me know if you're happy with the solution, and I'll add the dataset files to the repository. If prior
is actually the preferred choice, then I would create a repo with all the datasets (also the 2021 and 2022 ithor ones) and install all the data via an invoke
command calling prior.load_dataset
, if that sounds reasonable.
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 went ahead and prepared an installer for the ProcTHOR dataset. If the design seems fine, we could port the regular iTHOR ones in a similar way.
baseline_configs/one_phase/procthor/ithor/ithor_one_phase_rgb_base.py
Outdated
Show resolved
Hide resolved
baseline_configs/one_phase/procthor/ithor/ithor_one_phase_rgb_il_base.py
Outdated
Show resolved
Hide resolved
mp = mp.get_context("spawn") | ||
|
||
# Includes types used in both open and pickup actions: | ||
VALID_TARGET_TYPES = { |
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.
Can we instead use rearrange.constants.REARRANGE_SIM_OBJECTS
?
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 see this set is actually including 'Poster', 'RoomDecor', 'Window', 'Desktop', 'GarbageCan', 'Floor', 'BathtubBasin', 'PaperTowel', 'AppleSliced', 'Bed', 'PotatoSliced', 'Mirror', 'Dresser', 'Toaster', 'Desk', 'TargetCircle', 'StoveKnob', 'Sink', 'Television', 'ShowerHead', 'EggCracked', 'DiningTable', 'Shelf', 'SinkBasin', 'TVStand', 'Ottoman', 'ArmChair', 'LightSwitch', 'ShowerGlass', 'DeskLamp', 'VacuumCleaner', 'Curtains', 'CounterTop', 'StoveBurner', 'TowelHolder', 'Chair', 'Sofa', 'DogBed', 'BreadSliced', 'LettuceSliced', 'Painting', 'Bathtub', 'TomatoSliced', 'CoffeeTable', 'ToiletPaperHanger', 'GarbageBag', 'ShelvingUnit', 'Faucet', 'HandTowelHolder', 'HousePlant', 'SideTable', 'FloorLamp', 'Stool', 'CoffeeMachine'
, which are not potential targets given the action space, so I would not do that.
@@ -0,0 +1,1256 @@ | |||
"""Include the Task and TaskSampler to train on a single unshuffle instance.""" |
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.
Doing a diff between this and rearrange/tasks.py
suggests to me that we might (and probably should) use the same task for both projects and just have a few switches here and there that tell us what environment we should use and how we should load data.
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 slightly modified the original task and subclassed for ProcTHOR
. Not exactly the same task for both environments, but almost...
…gement-procthor into procthor-2022
Moved procthor-related modules as submodules of rearrange ones Removed redundant procthor_rearrange_constants Updated requirements and conda environment Doc updates + removed ai2thor_rearrangement prefix for data paths in inv tasks Typo Update README.md Fixed base dirs for ProcTHOR commands Fixed base config vo ithor_mini_valid eval config Update README.md Added TOC entry for ProcTHOR pre-training Doc update + bug fix in data path constant Missing ctx in call to split_data
Hi @jordis-ai2, @Lucaweihs I'm looking forward to work with ai2thor-rearrangement challenge using PROCThor rooms dataset, can you please help me to understand how can I do so? Which branch can I use now? Can you please also share the example script to work with rearrangement one-phase or pwo-phase challenge using ProcTHOR dataset splits? Thank you a lot! |
Hi @mariiak2021. It's currently not possible to use the |
Hi @jordis-ai2 thank you for the fast answer! |
I think @mattdeitke or @Lucaweihs would have more accurate answers to these questions, but here's what I know: The currently missing dataset is composed of 10,000 ProcTHOR one and two-room houses, out of which we used 2,500 for our rearrangement experiments. This dataset is not yet available in our upcoming distribution package -as far as I know-. Once it is there and the ongoing PR is merged, our experiments should be repeatable. As mentioned above, this is planned for the next few weeks. |
Hi @jordis-ai2 , so sorry I've missed your reply! Thanx a lot for the update. :) Will wait the release, but in the meantime I played around with your code and created my own dataset from PROCThor-10K. I wanted to use it within the rearrangement challenge, but got a problem with instance_detections2D dictionary. This returns for me now only high level objects without children. To be more precise I guess I can see those objects inside instance_detections2D, which were not not "objects" (f.ex. "wall" or "door"). Those objects, which are affected by those lines:
don't appear at all in the instance_detections2D dictionary. |
Good job! In principle it should work just as well with those houses. If I understand it correctly, the problem you describe is a mismatch between the objects available under I assume you initialized THOR with I'm not sure which version of THOR you're using, but my guess is that instance segmentation might not be (fully) supported for ProcTHOR yet. |
Hi @jordis-ai2, In the metadata["objects"] the overridden IDs are shown: instead of 'id': 'Bed|2|0' we will see 'id': 'Bed_18_2_0' for example, which is the combination of 'assetId': 'Bed_18_2' + 0 as it's the only bed in the scene. I have tried different versions of Ai2Thor with different errors:
I have only:
Here bed is missing, as it was inside "objects", which IDs were changed. The funny thing is that if I don't use the fix_object_names function, instance_detections2D returns correct results.
Do you have any ideas what can I do?
|
First of all, if the new IDs are used for objects but not for detections without any error message, it seems to be a THOR issue, which will probably disappear once ProcTHOR is officially released. Please keep in mind you don't really need to access instance segmentation to solve the rearrangement task. Indeed, in the official challenge, the only available visual inputs are RGB and depth. I.e., for the intended scope of this PR, you can just leave the current ID override on (this is how our models were trained and evaluated). It's possible that you don't need to override object IDs any longer. With the original For the other questions, I'm afraid they're out of the scope of this PR. I don't want to repeat myself, but, if you can wait for a few weeks, you will be able to use the official release, where things will be less likely to suddenly change. In any case, I think you're in the right direction 👍 |
Hi @jordis-ai2 thank you for your answers! I really need instance_detection2D to get 2D bboxes for training my model. :) |
Merging procthor-rearrangement code into main.