ejhigson / dyPolyChord

Super fast dynamic nested sampling with PolyChord (Python, C++ and Fortran likelihoods).
http://dypolychord.readthedocs.io/en/latest/
MIT License
22 stars 5 forks source link

Documentation - Functionality documentation #4

Closed andrewfowlie closed 6 years ago

andrewfowlie commented 6 years ago

For my review, I am almost happy to check this requirement, as the API method documentation is generally really great.

Documentation

  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

I have two comments though:

  1. The names/location of the output files should be made clearer. E.g., the run_dynamic_ns docs don't give a clear indication of where the new results will be written and what they contain. A user unfamiliar with PolyChord etc might be confused.

  2. The code in many places favors the syntax

def myfunction(x, y, *kwargs)
    z = kwargs.pop('z', 10)
   ...

to

def myfunction(x, y, z=10)
    ....

I don't understand the preference for that? and it makes reading the docs and code trickier for me as I can't tell much by just glancing at the signature. But I think there must be a reason? and it's not a big enough deal for me to insist that it's changed.

ejhigson commented 6 years ago
  1. The names/location of the output files should be made clearer. E.g., the run_dynamic_ns docs don't give a clear indication of where the new results will be written and what they contain. A user unfamiliar with PolyChord etc might be confused.

I have added this (in the docstring of run_dypolychord).

ejhigson commented 6 years ago

2. The code in many places favors the syntax

def myfunction(x, y, *kwargs)
    z = kwargs.pop('z', 10)
   ...

to

def myfunction(x, y, z=10)
    ....

I don't understand the preference for that? and it makes reading the docs and code trickier for me as I can't tell much by just glancing at the signature. But I think there must be a reason? and it's not a big enough deal for me to insist that it's changed

I think there are some reasonable use cases, for example its quite clean when you have complex/interdependent default values (as for some arguments in run_dypolychord) or when you want to pass on a whole bunch of kwargs to a sub function. I also prefer kwargs with no default value to long lists of positional arguments to avoid errors in ordering, as in "process_initial_run" (I think its much cleaner than e.g. named arguments with "arg=None" then checking if they are None). However I think I have got into the habit of using it too much for no good reason and should use more named arguments. Hopefully with good docstrings this doesn't really matter. I will bear this in mind in future but don't really have time to change everything now (assuming you are happy with this).