spedas / pyspedas

Python-based Space Physics Environment Data Analysis Software
https://pyspedas.readthedocs.io/
MIT License
155 stars 60 forks source link

Issues with documenting wrapped/decorated functions #839

Open jameswilburlewis opened 6 months ago

jameswilburlewis commented 6 months ago

There are a few places where we use wrappers or decorators on functions that are meant to be called by users. For example, in MMS, the short load routine aliases fgm(), aspoc(), feeps() etc. are wrappers around mms_load_fgm, mms_load_aspoc, mms_load_feeps, etc. The load routines themselves are decorated with a @print_vars wrapper.

For GOES, there's a common goes.load() routine that handles all the instruments, and the short names fgm(), eps(), epead() etc. are defined as functools.partial wrappers around goes.load(). Each instrument wrapper specifying the appropriate instrument parameter to goes.load().

This can cause some minor problems with the sphinx documentation on pyspedas.readthedocs.io. The docstrings for goes.load() contain a disclaimer that it's not meant to be called by end users, but the disclaimer shows up in the readthedocs entries for the short names. Also, For the print_vars wrapper in the MMS code, any sphinx formatting errors in the docstrings show up in the build logs as errors in print_vars(), so you lose the information about where the real error was.

In both Pycharm and readthedocs, for the MMS short names, the generic wrapper arguments (args, kwargs) are shown, rather than the arguments of the underlying wrapped function mms_load_fgm(), etc.

I think print_vars() can probably be eliminated. The load routines return the list of loaded variables, so the user can print it if they really want to see it.

The MMS short names are annoying to work with, but maybe Pycharm will fix this some day.

For GOES, there is value in only having to write the docstring once for goes.load(), but we should probably rewrite it to avoid the disclaimer showing up misleadingly in the readthedocs entries, and add notes in the goes.rst that these are implemented as wrapper functions and you don't need to pass the instrument argument.

jameswilburlewis commented 3 months ago

There's also the issue that partial() definitions don't show up in libs() output....perhaps because they're objects, not functions?

jameswilburlewis commented 3 months ago

With the better_partial() and update_wrapper() tools now available, I think this issue is more or less moot. For MMS, I think we can replace the wrapper function definitions with simple assignments, achieving the same effect.