Fix plotter module (#167) #172
Conversation
1. The designer class in formatters.py is used for 3D graphing, but the default label list attribute only includes two axes. This causes an index out of range error. I added "z-axis". 2. The delta attribute of the Mesher class in formatters.py is 0.001. This is too detailed and can lead to mesher objects that are large enough to cause memory errors. I changed it to 0.1. 3. The use can't choose a colormap when graphing, so I added a colormaps property to the Designer class in formatters.py to allow the user to choose whichever one they want. It uses the same style as other properties of the class, and the module imports matplotlib.cm to validate both types of colormaps in matplotlib. I edited plotters/plot_countour to initialize a Designer object with a default colormap if none is provided. I've tested with every category of colormap as well as custom colormaps and verified that it is working as intended.
Default value for attribute limits in Designer class only has two dimensions. Added a third.
Understood.
This is good! I guess it's better to add flexibility with respect to colormaps. 👍
Copy that. Let me do the review in a while 👍 |
ljvmiranda921
added
bug
| @@ -10,6 +10,7 @@ | |||
| import numpy as np | |||
| from attr import attrs, attrib | |||
| from attr.validators import instance_of | |||
| from matplotlib import cm | |||
|
|
|||
There was a problem hiding this comment.
I think the best way to validate the colormaps is to check if they are instances of the matplotlib.colors.Colormap class. Both ListedColormap (the type of cm.viridis) and LinearSegmentedColormap (the type of cm.binary) are subclasses of Colormap.
So my suggestion is:
from matplotlib import cm, colors
class Designer(object):
colormap = attrib(
validator=instance_of(colors.Colormap),
default=cm.viridis,
)Can you confirm if this works? 😄
If this works, I think there's no need to write tests for all colormaps since we have a validator that gives out an error whenever we enter a non-Colormap
There was a problem hiding this comment.
That makes sense! I had been trying to validate colors.Colormap with type(), but that's redundant with attrib and messes things up.
| ) | ||
| colormap = attrib( | ||
| validator=instance_of((type(cm.viridis), type(cm.binary))), | ||
| default=cm.viridis, | ||
| ) |
There was a problem hiding this comment.
This line of code is related to my comment above. I think it's better to check against matplotlib.colors.Colormap because it is the base class for whatever type(cm.viridis) and type(cm.binary) is
Have the Designer class validate for the base class matplotlib.colors.Colormap instead of the individual types of colormaps.
|
LGTM! Will merge in a while 👍 |
|
Merged. Thank you so much @jayspeidell ! I appreciate this contribution! You're correct, I realized that we should decrease the resolution in the mesh to accommodate other machines. I also liked the fact that you opened-up flexibility for the users in choosing their colormaps. Good job! 🎉 Thanks a lot for this wonderful PR. I hope you enjoyed despite the minor hassle of separating PRs the last time hehe 💯 Aaron will take over the review of your other PR, we hope to see you again! |
|
@ljvmiranda921 Awesome, thanks! I appreciate the opportunity to learn the process. |
Description
Bug fix: Added 'z-axis' to formatters/Designer attribute 'labels' to prevent index out of range error when plotting in 3D. Also added a third dimension to 'limits' for the same reason.
New feature: I added a colormaps property to the Designer class in formatters.py to allow the user to choose whichever one they want. It uses the same style as other properties of the class, and the module imports matplotlib.cm to validate both types of colormaps in matplotlib. I edited plotters/plot_countour to initialize a Designer object with a default colormap if none is provided. I've tested with every category of colormap as well as custom colormaps and verified that it is working as intended.
Default parameter change: Mesher.delta from 0.001 to 0.1. This makes it safe to run many of the functions on an average computer. With 0.001, the mesher object grows very quickly.
Related Issue
Fix plotter module (#167)
Motivation and Context
Adding a colormap attribute to Designer made easier to verify the functions. I implemented because it enabled me to use discreet colormaps.
How Has This Been Tested?
Then I used flask8 and black. I restricted flask8 and black testing to the files that I edited in each commit. I also ran py.test.
I tested Designer on all types of matplotlib colormaps. I didn't add a test to do this automatically, but it will return a TypeError if it's not a valid colormap. Does it need a test?
Screenshots (if appropriate):
Types of changes
Checklist: