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

Move matplotlib import to the public function that uses it #91

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

rafmudaf
Copy link
Collaborator

Importing matplotlib for the entire module causes an error if matplotlib is not available. The plotting functionality is not currently supported by NREL so it should remain optional. Moving the import statement to a function moves the failure to that function call rather than the module import.

With this pull request, if matplotlib is not available and plotting is off, the result summaries in html format are still exported correctly. If matplotlib is not available and plotting is on, the html files are written but the plotting routine fails.

When running the regression test through CTest, this python ModuleNotFoundError fails silently. Users can see the error with the verbose flag in the CTest call: ctest -V

This fixes issue #83 .

@rafmudaf rafmudaf self-assigned this Feb 20, 2018
@sayerhs
Copy link
Contributor

sayerhs commented Feb 20, 2018

@rafmudaf Why not use check and gracefully disable plotting?

try:
    import matplotlib
    matplotlib.use("Agg")
    import matplotlib.pyplot as plt
    from matplotlib.ticker import FormatStrFormatter
    have_plt = True
except ImportError:
    have_plt = False

# and in function 

if not have_plt:
    return

@rafmudaf
Copy link
Collaborator Author

@sayerhs If a user doesn't realize matplotlib is not installed and attempts to plot, this gives no indication of the failure. My update prevents a crash when plotting is off and matplotlib is not available, but maintains the Python exception reporting.

@sayerhs
Copy link
Contributor

sayerhs commented Feb 20, 2018

Well, just print a message instead of a return

if not have_plt:
    print("Matplotlib is not installed on this system; plotting functions will be disabled")

My point being that if plotting is optional it should not throw an exception ever. The user should just be informed that plotting is disabled.

@rafmudaf
Copy link
Collaborator Author

Is this functionally different than letting Python handle the exception?

@rafmudaf rafmudaf force-pushed the bugfix/regression_test_module_import branch from 931236a to 60ff2ae Compare February 20, 2018 19:05
@rafmudaf rafmudaf merged commit b0d4837 into OpenFAST:dev Feb 20, 2018
@rafmudaf rafmudaf deleted the bugfix/regression_test_module_import branch February 20, 2018 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants