heplesser
commented
2 years ago The actual import of Matplotlib happens at the beginning of hl_api_spatial.py and is properly protected
https://github.com/nest/nest-simulator/blob/7086c3cabb8923efd2ca783da02e8253ecfde865/pynest/nest/lib/hl_api_spatial.py#L36-L42
HAVE_MPL is set according to the result of the import attempt. I am not sure why pyplot is not imported here.
In the three Plot... functions, we should give a sensible error message, as suggested by @hakonsbm.
According to a comment right ahead of the code snippet in the issue description
https://github.com/nest/nest-simulator/blob/7086c3cabb8923efd2ca783da02e8253ecfde865/pynest/nest/lib/hl_api_spatial.py#L928-L930
the renewed import in the functions happens to avoid users changing backends. @Silmathoron, was that a real problem and how does this approach help? I'd prefer to move the import to the front of the file. If this approach is really needed, the import should certainly be under the if HAVE_MPL:
github-actions[bot]
hakonsbm
As pointed out in https://github.com/nest/nest-simulator/pull/2394#discussion_r914561320, the checks of
HAVE_MPLin hl_api_spatial.py only give a slightly nicer error message than the standard import error. Furthermore, the checks will never run, because they are after the import statement, see for example https://github.com/nest/nest-simulator/blob/7086c3cabb8923efd2ca783da02e8253ecfde865/pynest/nest/lib/hl_api_spatial.py#L930-L933The checks of
HAVE_MPLshould therefore be removed in these functions. Alternatively, if we want a more helpful error message (such as"The function foo requires matplotlib, but matplotlib could not be imported"), the check should be moved before the import statement in each function.