image-dreamer icon indicating copy to clipboard operation
image-dreamer copied to clipboard

Cleanup; Add `main`. Change CLI parser to use Python stdlib's `argparse`. Configuration file.

Open tildebyte opened this issue 9 years ago • 7 comments

  • PEP8!
  • Move most end-of-line comments above their related code.
  • Copyedit most comments.
  • Add if __name__ = __main__: so that we're a proper command line program with a single point of entry and (mostly) single point of exit.
  • Add nice command line argument parsing/args help using Python stdlib's argparse module

I came up with this patch independently of @Tursaanpoika's nice work in #8. I think a proper main is always appropriate for a stand-alone command line script. @donaldm changes in #7 amount to a fork, IMHO. Also, I'm not sure why it's a good idea to use JSON (complex) over Python stdlib's ConfigParser (simple), or use the (almost) deprecated and more complicated optparse over argparse.

I'm looking next at pulling out the model vars (model_path, net_fn, param_fn) into a ConfigParser file, as I agree with @donaldm that it'd be nice for them to be easily configurable, and that it's a bit too involved for the command line. Also, it might be nice to option-ize the output image format and mention in the --input help some of the other formats which PIL can handle.

tildebyte avatar Jul 15 '15 15:07 tildebyte

I think @donaldm https://github.com/Dhar/image-dreamer/issues/7 had one more option in the code, scale. I'll grab your source and tinker around a bit :D argparse is what google came up when I searched on how to add cli -options to python, I'm no developer, but I still do like to nose around to see what I can do :)

Tursaanpoika avatar Jul 15 '15 16:07 Tursaanpoika

Oh, it was step scale, defaulting to 1.5

Tursaanpoika avatar Jul 15 '15 16:07 Tursaanpoika

Right, I just spotted that myself and restructured things a bit.

tildebyte avatar Jul 15 '15 18:07 tildebyte

I opted to use JSON because I could represent the image mean list could be directly represented in the JSON instead of having to parse it out. Also I think JSON is a nice format which can support additions to the format.

I used OptionParser because I usually have to work with/support Python 2.4 (lol). I am just more familiar with it and I really don't see what argparse adds that I needed for this (but argparse works just as well :)).

I do agree my version is more of a fork though as I restructured things a good bit from the original code.

donaldm avatar Jul 16 '15 01:07 donaldm

Awesome. Thanks for the comments. I have a ConfigParser commit almost ready. For this usage, it's really simple; it's also really easy to extend. I really like the readability of the cfg file itself.

I agree that parsing complex data could be a pain; I ran into the exact issue with mean which you mentioned ;)

tildebyte avatar Jul 16 '15 02:07 tildebyte

Are you using ConfigParser or SafeConfigParser?

Also I think the concept of mean needs to be looked at, because there are mean files potentially in the models themselves that are not being used currently. May need to evaluate this.

donaldm avatar Jul 16 '15 02:07 donaldm

Safe.

tildebyte avatar Jul 16 '15 11:07 tildebyte