Skip to content
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

Change from "_" to "-" #821

Closed
darkosarajkic997 opened this issue Mar 28, 2022 · 3 comments
Closed

Change from "_" to "-" #821

darkosarajkic997 opened this issue Mar 28, 2022 · 3 comments

Comments

@darkosarajkic997
Copy link

darkosarajkic997 commented Mar 28, 2022

In commit f0bb84d arguments names are change to be compliant to GNU standards but they didn't change to new argument names in any place where args object that is returned from function is used.

@wangruohui
Copy link
Member

The argparse library will do this internally. For example, '--a-b' will be parsed as args.a_b automatically.
Does this cause some bugs on your side? Would you please provide a case for us to reproduce?

@darkosarajkic997
Copy link
Author

When I tried to run demo/restoration_video_demo.py script a few days ago after that commit I couldn't make it work, but for some reason when I try it now it works. I tested it out again and demo scripts still doesn't work on that specific commit. I created short Colab notebook where I tested this: Open In Colab
I get following error message:

Traceback (most recent call last):
File "/content/mmediting/demo/restoration_video_demo.py", line 86, in
main()
File "/content/mmediting/demo/restoration_video_demo.py", line 62, in main
output = restoration_video_inference(model, args.input_dir,
AttributeError: 'Namespace' object has no attribute 'input_dir'

And if I go to demo/restoration_video_demo.py file and change every argument parameter from "-" to "_" then this code works.
So somehow between that commit and current state of project this error got fixed, because if I try same code without checking out to this specific commit, it works.

Main thing is that it works now but I was really confused because it looked like pretty obvious mistake

@wangruohui
Copy link
Member

Oh yes. That modification did introduce some bug. We change all parameters from underscore to hyphen, but argparse will only convert optional parameters in Python, thus required args becomes buggy. But we fixed it in #822. Now this feature should work as previous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants