Skip to content
This repository has been archived by the owner on Jan 25, 2021. It is now read-only.

Clean up documentation #11

Open
goerz opened this issue Apr 19, 2019 · 7 comments
Open

Clean up documentation #11

goerz opened this issue Apr 19, 2019 · 7 comments
Labels
documentation corrections or additions to the documentation help wanted

Comments

@goerz
Copy link
Member

goerz commented Apr 19, 2019

The documentation is currently not in great shape, with lots of errors being reported, and the corresponding glitches in the generated HTML.

make docs 2> docs.log

docs.log

All of these need to be fixed, and every HTML file in the API should be checked manually for readability. Some docstrings may not fully match the requirements of the Google Style. The krotov package (whose API docs are in perfect shape) provides good examples for what to do, if in doubt.

These fixes are in addition the some more in-depth sections in the documentation that explain concepts and connections, beyond just the API. Writing these long-form documents would benefit significantly from #8, allowing to make connections to the QDYN documentation.

@goerz goerz added help wanted documentation corrections or additions to the documentation labels Apr 19, 2019
@ablech
Copy link
Member

ablech commented May 3, 2019

Shall we add a description of the arguments and return values to function docstrings?

@goerz
Copy link
Member Author

goerz commented May 4, 2019

Yes, please. Anything that has missing or incomplete docstrings should be completed.

@MatthiKrauss
Copy link
Member

I just saw, that functions, which are vectorized via np.vectorize do not appear in the documentation... One example is the flattop function in the pulse module. It also seems, that these functions are not even mentioned if one calls python's native help() function. I guess that, it might be due to the reason, that after the vectorization, the functions is a <numpy.lib.function_base.vectorize at 0x????????????>. I also haven't really found something on the internet about this! Should we maybe vectorize these function by hand?

@goerz
Copy link
Member Author

goerz commented Jun 21, 2019

Interesting... That sounds like it should be reported as a bug to numpy. Maybe we'll have to work around this by having our own vectorize wrapper that calls numpy.vectorize, but also preserves docstrings?

@MatthiKrauss
Copy link
Member

MatthiKrauss commented Jun 24, 2019

Actually numpy.vectorize seems to preserve docstrings, but the object it returns is no longer 'part' of the module. The returned wrapper is rather associated with numpy (help(vectorized_function) returns e.g. the help for vectorize). That's also the reason why I am a little hesitant to report this as a bug...

One possible (hot)fix for this could be something like:

@numpy.vectorize
def _mod_func_notvectorized(arg1,arg2,...):
  do_something()
  ....

def mod_func(*args, **kwargs):
  """ actual docstring """
  return _mod_func_notvectorized(*args, **kwargs)

But this does not seem very elegant

@goerz
Copy link
Member Author

goerz commented Jun 24, 2019

Definitely a bug in numpy, in my opinion (numpy/numpy#13826), although I'm not overly optimistic that this will get fixed (as they can arguably shift the blame to pydoc/Sphinx, and they might consider it a "breaking change").

In principle, we could add something like the following to qdyn.linalg as a replacement for np.vectorize:

def vectorize_function(pyfunc, **kwargs):
    """Function decorator acting like :class:`numpy.vectorize`.

    All arguments are passed to :class:`numpy.vectorize`. Unlike
    :class:`numpy.vectorize`, this decorator does not convert the decorated
    function into an instance of the :class:`numpy.vectorize` class. As a
    result, the decorated function's docstring will be taken into account
    properly by doc generators (pydoc or Sphinx)
    """
    wrapped = np.vectorize(pyfunc, **kwargs)
    @functools.wraps(pyfunc)
    def run_wrapped(*args, **kwargs):
        return wrapped(*args, **kwargs)
    run_wrapped.__doc__ = wrapped.__doc__  # preserve np.vectorize's `doc` arg
    return run_wrapped

The name vectorize_function is so that it doesn't clash with the existing vectorize function that we have to vectorize matrices into vectors. Since we're only using np.vectorize in two places in the pulse module, this might not be worth the effort, though: probably easier to catch array arguments and handle them internally: Something like if isinstance(t, np.ndarray): return np.array([flattop(t_val, t_start, t_stop, t_rise, t_fall) for t_val in t]), in flattop, for example.

Let's just avoid using np.vectorize going forward (and, a good lesson: never use classes to implement function decorators)

@MatthiKrauss
Copy link
Member

ok, we'll see what happens. Thanks for the numpy issue!
Although the vectorize_function looks nice, I am also rather in favour of rewriting the mentioned functions. Especially, since there are only a few...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation corrections or additions to the documentation help wanted
Projects
None yet
Development

No branches or pull requests

3 participants