-
Notifications
You must be signed in to change notification settings - Fork 46
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
Adding a plotting switch and changes made as suggested by So-Cool. #21
base: master
Are you sure you want to change the base?
Conversation
@greninja I had a look at your pull request. Moreover, on lines 803 and 1146 of Finally, could you please squash all the commits into one. Much appreciated! Thanks for the contribution BTW. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than piling up new commits, could you please revert these two files to their original form. |
Is it fine if I just close this PR and make a new one as this is getting a bit messy? |
Please fix this one. It should be fairly simple: check out these two files to their original commit and squash all the other. |
commit |
No problem @greninja. We're heading in the right direction. e214147 looks good except that it removes lines 803 and 1146 rather than introducing lines 810--813 and 1156--1158 directly. Moreover, this pull request is still 10 commits, please try to make it only 1, i.e. e214147 with the changes I've mentioned above. |
Closing this PR. Had some issues with local repo. Will open it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the comments below.
modules/processing/cuckooml.py
Outdated
import matplotlib.pyplot as plt | ||
import seaborn as sns | ||
except ImportError, e: | ||
print >> sys.stderr, "Some error while importing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningful error message would be nice, e.g. "Plotting libraries (matplotlib and seaborn) are missing.".
modules/processing/cuckooml.py
Outdated
# Safety check for plotting | ||
if not Config("cuckooml").cuckooml.plotting and figures: | ||
print >> sys.stderr, "Warning: 'plotting' and 'figures' do not match. \ | ||
Plotting modules might not be imported." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningful error message would be nice here as well. It should tell the user that plotting is disabled (globally) in config file and that the local plotting flag will not have any effect.
Also, one new line is enough.
modules/processing/cuckooml.py
Outdated
if not Config("cuckooml").cuckooml.plotting and plot: | ||
print >> sys.stderr, "Warning: 'plotting' and 'plot' do not match.\ | ||
Plotting modules might not be imported." | ||
plot = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaningful error message would be nice here as well. It should tell the user that plotting is disabled (globally) in config file and that the local plotting flag will not have any effect.
Also, one new line is enough.
709f52f
to
f7dc849
Compare
Have defaulted the global variable "imported" to true and it will be set to False when the imports fail. Is it fine? |
The Finally, please keep indentation clean and tidy. |
Referring to your third line, @So-Cool : Firstly, even if the global variable is set to Secondly, I found another loophole in this approach. The global variable asserts whether the plotting libraries are available or not. I can add an So in conclusion, in the case where config variable is disabled , the global variable will mislead the user into believing that the libraries are not available, which may not be true always. Will clean the indentation and thanks for the link! |
The variable is not telling whether the libraries are available but whether they are imported. That's why:
makes more sense. With you approach if plotting is disabled in config this variable will still be Then, since
therefore it's easy to understand. Please let me know if you have any other questions. |
No description provided.