vandal-vpr / vg-transformers

Official Repository of "Learning Sequential Descriptors for Sequence-based Visual Place Recognition "
MIT License
39 stars 5 forks source link

code problem #13

Closed Steven-jiaqi closed 1 year ago

Steven-jiaqi commented 1 year ago

I found this function get_aggregator(args) in tvgnet.py 82 lines. But there is no args.aggregation ''. And i found the args.features_dim=768 in the function get_encoder(args). So I assign the args.features_dim =768. But new error appeared: ValueError: optimizer got an empty parameter list

I can't solve this problem. I need your help. I'm sorry to bother you again. Looking forward to get your reply as soon as possible!

ga1i13o commented 1 year ago

Hello, I think I solved your issue, and it is actually my fault. The problem is at this line:

https://github.com/vandal-vpr/vg-transformers/blob/25537ed1689d4b5401113f6c4476fb8f0cf28158/tvg/models/tvg_net.py#L159

In the README I indicated that you should pass --arch timesformer, but the code actually expects you to pass --arch Official-timesf, as you can see in the line above. I will fix this as soon as I can and for now you can change your command line args to --arch Official-timesf

Thank you for noticing. I am closing your other issue

Steven-jiaqi commented 1 year ago

Thank you for timely response.And I have another question. Is the --aggregation 'none' or ''? It seems to be '' in the code. Please forgive me for having a bit too many questions! Best wishes to you!

ga1i13o commented 1 year ago

Don't worry, I should have done a better job documenting the code. Anyway for the TimeSformer you can pass either 'none' or leave the field empty, since in that case only the encoder will be called as you can see in the forward method (using timesformer sets fusion='early ' )

https://github.com/vandal-vpr/vg-transformers/blob/25537ed1689d4b5401113f6c4476fb8f0cf28158/tvg/models/tvg_net.py#L45-L49

Steven-jiaqi commented 1 year ago

I have solved the problem. I'm sorry to bother you.

ga1i13o commented 1 year ago

Yes the issue should have been that you have to specify the image resolution. I am working to push a commit to make this automatic, and update the readme