search

baudren / montepython_public

Public repository for the Monte Python Code
MIT License
65 stars 115 forks source link

Continuation of improvement on the command line parser #12

Closed montefra closed 10 years ago

montefra commented 10 years ago

This pull request continue the PR #7.

montefra commented 10 years ago

@baudren : I've implemented the run and info subparsers. Except for the two subparsers, the dest of the various options has not been changed. So If I've caught all the places where the choice between run and info is done, there should be no problem in running montepython in any of the two configuration. All the tests pass. If anyone wants to test this branch (@jngrb ?), I'm happy fix bugs that I likely introduced. I'm watching the repo, so I should get all the issues/PR.

Further improvements I'll ponder about:

baudren commented 10 years ago

Wow, lots of work. I was doing a modification to implement MPI runs with Monte Python, so I had no time to look at it. I will review the changes now, and check-out your branch. I will try to merge it before the next version, if I see that everything works. Thanks again,

baudren commented 10 years ago

As a first comment looking at the changes, @montefra, do you think there could be a way to make the run subsampler the default one?

Otherwise, everyone would have to adapt their scripts to launch the code, and it might be a pain. I agree that this separation makes things clearer, but to simply maintain a sort of continuity, it would be nice to support the old syntax.

Could you point me towards a way of doing it, since you've implemented all that? It would be easier than you adding a new commit to your branch - as I had problems merging your branch, since I also modified the tests and parser (tabs problems...)

Thanks in advance

montefra commented 10 years ago

@baudren glad to be of help.

I have a question: if no rerun is requires, must the folder directory be passed to command line and existing? From the last else in parser_mp.py it would seem that it gets created if does not exists.

About making run the default subparser: I'll think about it. I think that I have an idea. I'll implement it in a new branch and do a new PR, so you can cherrypick what you need.

baudren commented 10 years ago

I have a question: if no rerun is requires, must the folder directory be passed to command line and existing? From the last else in parser_mp.py it would seem that it gets created if does not exists.

What do you mean by rerun? restart? Or simply creating a new chains in an existing folder? In any-case, if the folder exists and was successfully initialised, then the only needed argument is indeed the folder, with -o. The last else there: https://github.com/baudren/montepython_public/blob/master/montepython/parser_mp.py#L327-L351 corresponds to the case where the output folder is detected not be a folder, and no param file was asked.

I think this is the correct behaviour.

About making run the default subparser: I'll think about it. I think that I have an idea. I'll implement it in a new branch and do a new PR, so you can cherrypick what you need.

Ok - as long as you don't put more tabs in! :) I will also look on my side.

montefra commented 10 years ago

I have a question: if no rerun is requires, must the folder directory be passed to command line and existing? From the last else in parser_mp.py it would seem that it gets created if does not exists.

What do you mean by rerun? restart? Or simply creating a new chains in an existing folder? In any-case, if the folder exists and was successfully initialised, then the only needed argument is indeed the folder, with -o.

I meant restart, sorry

The last else there: https://github.com/baudren/montepython_public/blob/master/montepython/parser_mp.py#L327-L351 corresponds to the case where the output folder is detected not be a folder, and no param file was asked.

I think this is the correct behaviour.

Is the directory created in this case, isn't it?

About making run the default subparser: I'll think about it. I think that I have an idea. I'll implement it in a new branch and do a new PR, so you can cherrypick what you need.

Ok - as long as you don't put more tabs in! :) I will also look on my side.

I never use tabs (my .vimrc has set expandtab). You mean that I should try to use the indentation you use for long lines (like the ones when adding parameters)?

baudren commented 10 years ago

Is the directory created in this case, isn't it?

No - there is no way to create the folder if no parameter file is created. I just tested it, and it indeed returns an error, but does not create the folder.

For the tabs, I understand - they must have been the same one introduced by @jngrb, that I already killed locally. No worries then! But yes, the style of indent would be nice to respect. With vim, you can install flake8 and pylint, then the syntastic vim module, that will display where the errors are (hanging indents, trailing whitespaces, number of blank lines, etc...).

The only thing I don't follow from their recommendation is the local imports.

baudren commented 10 years ago

Ok, done! I implemented a modified function parse_args, called safe_parse_args, that adds the default subparser. Apparently there is no default behaviour to implement this within argparse.

make the parser complain if the output folder passed by -o FOLDER does not exists

@montefra, do we agree that this is not correct?

montefra commented 10 years ago

Ok, done! I implemented a modified function parse_args, called safe_parse_args, that adds the default subparser. Apparently there is no default behaviour to implement this within argparse.

I was planning to do it on my way home. You won :+1:. My idea was to redefine the parse_args, in MpArgumentParser such that it checks if either run or info exists. If not, prepend run and call super.parse_args (or whatever is the correct syntax). I guess that you've basically done this, right?

make the parser complain if the output folder passed by -o FOLDER does not exists

@montefra, do we agree that this is not correct?

we do. That was my misunderstanding.

baudren commented 10 years ago

Yup, that's what I did - while checking that no -h, --help nor --version is asked - otherwise the behaviour is bad. So, in next release. Thanks for the exchange!

baudren commented 10 years ago

Hum, I am still faced with two problems.

montefra commented 10 years ago

@montefra, do you have a nice solution so that when one does: MontePython.py -h, a message says explicitely to do MontePython.py run -h or MontePython.py info -hfor further information? It is not clear from the message (one is tempted to doMontePython.py -h run`). But if I append to the help of the parser, it concatenates the string without respecting the blank lines, making it ugly.

  • option 1:
parser = MpArgumentParser(
    formatter_class=argparse.ArgumentDefaultsHelpFormatter,
    description='Monte Python, a Monte Carlo code in Python',
    epilog='''Type "%(prog)s run -h" or "%(prog)s info -h" for further
    information''')

prints

usage: MontePython.py [-h] [--version] {run,info} ...                                                                                                                                                                            

Monte Python, a Monte Carlo code in Python

positional arguments:
  {run,info}
    run       run the MCMC chains
    info      analyze the MCMC chains

optional arguments:
  -h, --help  show this help message and exit
  --version   show program's version number and exit

Type "MontePython.py run -h" or "MontePython.py info -h" for further
information
usage = """%(prog)s [-h] [--version] {run,info} ... """
usage += textwrap.dedent("""\n
                From more help type: 
                %(prog)s run -h
                %(prog)s info -h""")

parser = MpArgumentParser(
               formatter_class=argparse.ArgumentDefaultsHelpFormatter,
              description='Monte Python, a Monte Carlo code in Python',
              usage=usage)

prints:

usage: MontePython.py [-h] [--version] {run,info} ... 

From more help type: 
MontePython.py run -h
MontePython.py info -h

Monte Python, a Monte Carlo code in Python
[...]
baudren commented 10 years ago

Wow, thanks. I had already implemented version 1, but your 2nd option beats it out of the water. Great trick! It is in devel,now. Only the bug left

montefra commented 10 years ago

There is a bug, introduced by these changes, where the code fails to find the file.conf even when it is not needed (i.e. when restarting in an existing folder). It is a devious bug, since it appears only when launching a job on a cluster... If you have any clue, let me know!

if it's caused by this option:

# -- configuration file (OPTIONAL)
runparser.add_argument('--conf', help='configuration file',
      type=existing_file, dest='config_file', default='default.conf')

just remove type=existing_file or default='default.conf'. Remove the first if you want that config_file always defaults to default.conf (even when the file does not exist). Remove default if you want the to check the existence of the config any time --conf is used

montefra commented 10 years ago

Wow, thanks. I had already implemented version 1, but your 2nd option beats it out of the water. Great trick! It is in devel,now. Only the bug left

Thanks :D

the only problem could be that you need to adapt by had usage if new subparser are added (but I guess that will be quite unlikely)

baudren commented 10 years ago

One less bug on the surface of this earth. The problem was there https://github.com/montefra/montepython_public/blob/parser/montepython/parser_mp.py#L244-L245 Testing for an existing file at this point assumes that you are indeed launching the code from the repository where default.conf lives, which is not a given. It is fixed in devel, now, and I guess it was the last bit missing for the release.

baudren commented 10 years ago

Rah, there was still one bug! When you changed the default values of the ticksize and fontsize, this killed the behaviour of the analyze module. I was choosing the value "-1" to use the scaling relation, and otherwise, a specified parameter. When you put instead different default values, this part of the analyze was never read, producing ugly plot. I guess I will do a fast new release to fix it.

montefra commented 10 years ago

ok. Sorry for that. I set the default according to the comments in the source code. If you change back to -1 change also the comments and the correspoding docstring.

btw: what do you mean with scaling relation? Do you scale the line tick and font sizes according to the figure/subplots sizes?

baudren commented 10 years ago

I use an ad-hoc scaling relation, because of the way the figure size is implemented. Generally, I don't really like it, but it works: The more the parameters you analyze, the smaller are the the plots, and so the font/tick size must be scaled down. We found a formula that works relatively well (when you zoom in on the plot, it is readable).

Thanks for the point on the documentation, will fix that as well.

montefra commented 10 years ago

I've found two tiny things. Since they are so small, I guess it's easier if you do them yourself

  1. typo here: From --> For
  2. adapt to new parser: should be --conf