search

paulknysh / blackbox

A Python module for parallel optimization of expensive black-box functions
MIT License
439 stars 60 forks source link

Formatting and docstring changes #1

Closed lancastdw closed 7 years ago

lancastdw commented 7 years ago

Hello, thank you for publishing your project! I'm no optimization expert, but I was interested in learning about your parallel optimization procedure.

I reformatted the code into a more conventional python style (see the scipy.optimize module for example). This will make the code more readable to the average user. I followed the numpy documentation guide for doctrings (https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt).

I also added a script (example.py) with an example from your arXiv note.

Best regards, Drew

paulknysh commented 7 years ago

Hi Drew,

Thanks for this amazing work! Didn't know that someone might be interested in this topic enough to refactor the entire thing :)

I'm most likely to accept your changes, let me just go over them sometime on the weekend or so (did you test them?). Also, I know nothing about git (I published this code simply via github web interface) so hopefully I'll press correct buttons to merge it.

Btw, are you a scientist too?

Paul

On Thu, Nov 3, 2016 at 9:19 PM, lancastdw notifications@github.com wrote:

Hello, thank you for publishing your project! I'm no optimization expert, but I was interested in learning about your parallel optimization procedure.

I reformatted the code into a more conventional python style (see the scipy.optimize module for example). This will make the code more readable to the average user. I followed the numpy documentation guide for doctrings (https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt).

I also added a script (example.py) with an example from your arXiv note.

Best regards,

Drew

You can view, comment on, or merge this pull request online at:

https://github.com/paulknysh/blackbox/pull/1 Commit Summary

  • Refactor search()
  • Reformat doctrings and code.
  • Add example script "example.py".
  • Add .gitignore.
  • Update example.py
  • Update example.py
  • Fix typos.
  • Minor docstring fixes.

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/paulknysh/blackbox/pull/1, or mute the thread https://github.com/notifications/unsubscribe-auth/APaxImTzxAOUHC8jcQ1Mj8FDajn4T_6Vks5q6oglgaJpZM4KpF6B .

lancastdw commented 7 years ago

You're welcome! Nice work!

Yes, I tested them using example.py. Since there are no conflicts you could simply click "merge pull request" after reviewing (https://help.github.com/articles/merging-a-pull-request/).

You may also want to add a license, test suite, requirements.txt, and setup.py file. See this guide for structuring and distributing a python project: http://docs.python-guide.org/en/latest/writing/structure/. I might be able to help with this if you're interested.

I'm an engineer (structures).

Drew

paulknysh commented 7 years ago

I'm merging. Few comments:

1) Again, nice job on being consistent with code formatting.

2) I'll need to double check input/output types of arrays in docstrings. For example, you're using all of the following types: list, array_like, ndarray. I guess it's better to call all of them 'list', but I remember sometimes there is a difference between passing a Python list or a numpy array..

3) I don't know if you noticed, but often times I'm writing a code like this:

pts = np.ones((n, d))

for i in range(n):
    pts[i] = pts[i]*i/(n-1.)

What I don't like about this is that there are 2 steps: initialization + for loop (sometimes double loop). Is there any construction that allows to initialize array like that in 1 step? This is not really a big deal, just wondering if there is a standard for such thing.

4) I like to keep it clean so I think I'll delete gitignore file (unless there is a reason I must have it) and I'll move your updated example into README file. I'm not going to add tests/setup etc. at the moment, I think it's pretty obvious how to use the module. I might add a license though (again, not sure how critical it is).

Paul

lancastdw commented 7 years ago

2) I agree this needs some work. You often see "sequence" or "array_like" in doctrings when several types are allowed. The "box" parameter is actually a list of 2-element lists in your example, but tuples may be preferable in this case. I like this example from scipy.optimize.brute():

 ranges : tuple
        Each component of the `ranges` tuple must be either a
        "slice object" or a range tuple of the form ``(low, high)``.
        The program uses these to create the grid of points on which
        the objective function will be computed. See `Note 2` for
        more detail.

3) Something like this would be better (a one-line expression wouldn't be very readable):

column = np.linspace(0, 1, n)
matrix = np.array([column]*d).transpose()

However, constructing a repetitive array like this usually isn't necessary (https://docs.scipy.org/doc/numpy/user/basics.broadcasting.html).

4) A license is pretty important since technically no one should be using or modifying code without a license.

Tests are important for building confidence that the code is correct. The unittest module makes automated testing fairly easy. I'll see if I can come up with a simple example. Of course, the level of effort you put in depends on how the code will be used (it's also just a good habit to get into). You'll probably want to refactor large functions into smaller functions to make them easier to test.