Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

f2py.compile issues (#7683) #7791

Merged
merged 1 commit into from
Jul 18, 2016

Conversation

bertrand-l
Copy link
Contributor

  • DOC: add to f2py.compile's doctstring a description of the source_fn argument
  • MAINT: avoid warnings about deprecated imp.load_module in distutil.misc_util with Python 3

That leaves one issue in #7683 that I have not yet figured out:

Could not locate executable C:ProgramsPython35python.exe
Executable C:ProgramsPython35python.exe does not exist

Looks like some string escaping issue, but nothing jumped at me looking at distutil.exec_command.{exec_command,find_executable} and I don't have a windows machine to test on.

@@ -35,8 +35,11 @@ def compile(source,
additional parameters passed to f2py
verbose: bool, optional
print f2py output to screen
source_fn : str, optional
filename where the fortran source is dumped
Copy link
Member

@charris charris Jun 30, 2016

Choose a reason for hiding this comment

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

Could use a bit more explanation. I can't figure out what "dumped" means in this context ;) Should document the default, here and for the other optional arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point :)

    source_fn : str, optional
        name of the file where the fortran source is written
        before being compiled. The default is to use a temporary
        file with an extension provided by the `extension` parameter

@charris
Copy link
Member

charris commented Jun 30, 2016

Looks generally good. Could you also fix the other imp uses? there is one in numpy/_import_tools.py that could use the same fix as here, and another in numpy/testing/utils.py. Hmm, might make sense to put your import_module function in numpy/compat/py3k.py

@charris charris added this to the 1.11.2 release milestone Jun 30, 2016
@bertrand-l
Copy link
Contributor Author

Good point. Will do and then update the PR. Thanks for the review!

@charris
Copy link
Member

charris commented Jul 2, 2016

If you put it in py3k.py, npy_import_module might be a better name.

@bertrand-l
Copy link
Contributor Author

@charris I think i've addressed all your comments (apart from the context manager in npy_import_module - keeping try: ... finally: ... block because imp.find_module returns a file object). Thanks again!

"""
Import and return module

name : str
Copy link
Member

Choose a reason for hiding this comment

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

Needs the heading

Parameters
-----------------

See doc/HOWTO_DOCUMENT.rst.txt or other functions for docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@charris
Copy link
Member

charris commented Jul 9, 2016

Couple of style nits. Also, please squash the commits using numpy rebase -i HEAD~4. You can reorder the commits without problems (cut and paste) as long as they don't touch the same files, then squash them by replacing pick with s.

@bertrand-l bertrand-l force-pushed the feature/f2py-issues-7683 branch 2 times, most recently from 8adc2ac to e805f16 Compare July 13, 2016 02:15
@bertrand-l
Copy link
Contributor Author

All done, including the squashing. Also renamed compat.py3k.npy_import_module to compat.py3k.npy_load_module as it is closer to the name of the original python functions.

filedescriptor)
info_module = npy_load_module(fullname + '.info',
info_file,
filedescriptor)
Copy link
Contributor Author

@bertrand-l bertrand-l Jul 15, 2016

Choose a reason for hiding this comment

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

oops, will fix that indentation, sorry [DONE]

@charris
Copy link
Member

charris commented Jul 16, 2016

The new npy_load_module function needs .. versionadded:: 1.11.12 at the end of the summary, grep for examples. LGTM otherwise.

@bertrand-l
Copy link
Contributor Author

ok thanks (and sorry there was yet one more thing i forgot). npy_load_module now has a versionadded.

"""
Load a module.

.. versionadded:: 1.11.12
Copy link
Member

Choose a reason for hiding this comment

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

Typo, I don't think we are 12 yet ;), seems to be 1.11.2

@bertrand-l
Copy link
Contributor Author

@seberg, thanks. The versionadded is fixed now.

@charris charris merged commit 055056a into numpy:master Jul 18, 2016
@charris
Copy link
Member

charris commented Jul 18, 2016

Thanks @bertrand-l .

@charris
Copy link
Member

charris commented Jul 18, 2016

For next time, note that your commit message summary line is too long. Lines should be kept <= 72 characters.

@charris charris removed this from the 1.11.2 release milestone Jul 18, 2016
@bertrand-l bertrand-l deleted the feature/f2py-issues-7683 branch July 19, 2016 00:10
@bertrand-l
Copy link
Contributor Author

Thanks @charris for the review and merge (and sorry it took so many iterations and comments)

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.

3 participants