Skip to content

DOC: distutils: Add a docstring to show_config(). - #15429

Merged
eric-wieser merged 1 commit into
numpy:masterfrom
WarrenWeckesser:config-docstring
Jan 27, 2020
Merged

eric-wieser merged 1 commit into
numpy:masterfrom
WarrenWeckesser:config-docstring

Conversation

@WarrenWeckesser

Copy link
Copy Markdown
Member

Thanks to Sergey Kojoian for the original patch to create
the docstring.

Closes gh-9258.

Comment thread numpy/distutils/misc_util.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in the ``numpy.distutils.system_info`` module.
in the `numpy.distutils.system_info` module.

Should produce a link here

Comment thread numpy/distutils/misc_util.py Outdated
Comment thread numpy/distutils/misc_util.py Outdated
Comment thread numpy/distutils/misc_util.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe

Suggested change
Print information about BLAS and LAPACK libraries in use.
Print information about libraries NumPy was built against.

Am I right in thinking this indicates what numpy actually uses? Or does it indicate what rebuilding numpy would use?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change looks good to me. I don't know the answer to those questions; I was assuming it shows what is being used by the numpy that ran show_config(). I'll take a look at the system_info module now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth checking before talking my suggestion :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it searches the system for the available libraries. Seems like a better name would be something like show_system_info() (which matches the underlying class that contains the code that does the work) or show_available_library_info(). Anyway, I'll fix the text.

@eric-wieser

Copy link
Copy Markdown
Member

I think we're missing something here. np.show_config() shows the system info where numpy was built, which is cached at build-time using the function you document here.

Comment thread numpy/distutils/misc_util.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the function that this docstring is attached to.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show_config is the new name given to show when it is imported into the numpy namespace (in numpy/__init__.py). I don't know why it isn't called show_config in __config__.py.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't np.show_config == np.__config__.show? This file is not np.__config__.

@WarrenWeckesser

Copy link
Copy Markdown
Member Author

I think we're missing something here. np.show_config() shows the system info where numpy was built, which is cached at build-time using the function you document here.

Yes, thanks for the reminder. I lost that point for a bit.

@WarrenWeckesser

Copy link
Copy Markdown
Member Author

How about

Show libraries in the system on which NumPy was built.

for the one-line summary?

@eric-wieser

Copy link
Copy Markdown
Member

I think:

  • The function in __config__.py should contain that first line, and an explanation that it calls numpy.distutils.misc_util.show_config and a link
  • The docstring you've written can stay as is, after adjusting the first line to indicate it's about the current system.

@WarrenWeckesser

Copy link
Copy Markdown
Member Author

I updated the one line summary.

I'm not sure what the other change is that you are suggesting. __config__.py is the generated code, and it contains the function __config__.show. This function is added to the numpy namespace as show_config with the line

from numpy.__config__ import show as show_config

in numpy/__init__.py. show_config is the public name of the function, which is (presumably) why np.show_config() is used in the docstring, even though the name of the function in the generated code is show.

There is no numpy.distutils.misc_util.show_config.

@eric-wieser

Copy link
Copy Markdown
Member

Apologies, I did not expand the diff context enough to see this was part of a large string.

Thanks to Sergey Kojoian for the original patch to create
the docstring.

Closes numpygh-9258.
@WarrenWeckesser

Copy link
Copy Markdown
Member Author

I squashed the two follow-up commits, and it looks like shippable has come back to life, so the tests have finally all passed.

@eric-wieser, I'm not sure if there are still changes that you'd like to see.

@eric-wieser eric-wieser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion

@eric-wieser

Copy link
Copy Markdown
Member

@eric-wieser
eric-wieser merged commit c9cd209 into numpy:master Jan 27, 2020
@WarrenWeckesser
WarrenWeckesser deleted the config-docstring branch October 25, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

show_config() has no docstring and is not in the online docs.

2 participants