search

soyeonm / FILM

Official repository of ICLR 2022 paper FILM: Following Instructions in Language with Modular Methods
114 stars 27 forks source link

Is there a typo in the names of the processed instructions? #19

Open TopCoder2K opened 2 years ago

TopCoder2K commented 2 years ago

There are no files with names 'test_seen_appended.p' and 'test_unseen_appended.p' in models/instructions_processed_LP/, although it's clear that read_test_dict tries to load files with 'appended' in names.

TopCoder2K commented 2 years ago

Also, I want to mention some other strange places that I've encountered. Maybe someday they will be fixed.

  1. What is the purpose of this and this code? :)
  2. Why is this argument set to true? GT anything can't be used during testing.
  3. RGB images are saved incorrectly as they are in BGR format
  4. Saved FMM map is mirrored (or rotated?)
  5. The semantic policy model (UNetMulti) is always loaded from best_model_multi.pt but in the tutorial its best performing checkpoint is named new_best_model.pt (moreover, there is a typo in the tutorial since the mv's first argument is best_model_multi.pt model, not new_best_model.pt)
soyeonm commented 2 years ago

Hello,

Thank you for your keen eyes, @TopCoder2K ! I uploaded the 'test_seen_appended.p' and 'test_unseen_appended.p' in models/instructions_processed_LP/.

With regards to your questions 1~5,

(general answer: A lot of engineering went into this work; I tried to clean the code but there are still some remnants of it.. sorry. )

  1. They are some remnants of engineering - trying different thresholds and numbers in different circumstances. There is no purpose.

  2. Sorry for confusing you with this. This is another remnant of engineering, but you can ignore this. If you go through the code, you will notice that "args.ground_truth_segmentation" is not used for almost anything (and definitely not for segmentation at all). I just deleted this line.

3-4. For these, users can customize it as they want.

  1. "best_model_multi.pt" is the semantic policy model used in the paper, and the "new best model" is an even better model that we found with another starting seed after submitting the paper. Users can use what they want for this as well.
soyeonm commented 2 years ago

And thank you very much, @TopCoder2K !

TopCoder2K commented 2 years ago

@soyeonm, thank you for your fast response!

As for 5, should the README file be updated then? The command

mv Pretrained_Models_FILM/best_model_multi.pt models/semantic_policy/new_best_model.pt

should be 

mv Pretrained_Models_FILM/new_best_model.pt models/semantic_policy/best_model_multi..pt

since the model is always loaded from best_model_multi.pt, isn't it?

And if the README gets fixed, I also want to remind you about this issue.

soyeonm commented 2 years ago

@TopCoder2K , thanks very much once again. I changed the readme.

With regards to this issue, I will reply again soon! But I do believe it does work.. does it not?

TopCoder2K commented 2 years ago

With regards to https://github.com/soyeonm/FILM/issues/2#issue-1188383110 issue, I will reply again soon! But I do believe it does work.. does it not?

The https://github.com/soyeonm/FILM/issues/9#issuecomment-1124906829 states that it's necessary to use --data data/json_2.1.0 when running python models/train/train_seq2seq.py, not --data data/json_feat_2.1.0 as in README. And a 'thumbs up' from another user suggests that this solves the problem. Could you please check the solution and, if it is correct, update the README once again?