Adding PowerScale for PowerNorm #20355 #20532
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Hi @yacth. Thanks for contributing. As you can see, some of your tests are failing. To begin with, the test is complaining about formatting - clik on Linting/flake8 (pull_request) for more details. From the contributing documents it recommends to do the following to your code: which ensures formatting is done correctly. Secondly, its also complaining about needing tests for the added features, as per the website linked above:
Its also important to make sure you run some tests before to make sure your changes are not breaking other features, see testing. For the implementation details, you will have to wait for some of the other developers to review it. But it greatly helps to make sure the previous tests passed, so the conversation can focus on implementation itself. |
Thank you for your review. Actually since it is rewritting the original feature in another way I thought that letting the old tests should be good enough. |
|
@yacth if the old tests fail then you changed the behaviour with your changes and the new code needs to be fixed, or you need to argue that the changes you made are an improvement and rewrite the tests. |
|
I am working on the code, but I am stuck. I am trying some testings using a simple code : import matplotlib.scale as scale
scale.PowerScale(10, 0.5) I get the following error : TypeError: __init__() takes 2 positional arguments but 3 were givenWhich I don't understand. |
There was a problem hiding this comment.
I did the changes to solve the errors related to the PowerNorm calculations even if there is still the set_default_locators_and_formatters in the PowerScale class is still to be determined.
I had this error:
lib/matplotlib/tests/test_axes.py::test_pcolor_regression
But by running what is inside line by line on my local environment I don't have a failure, I don't get it:
from pandas.plotting import (
register_matplotlib_converters,
deregister_matplotlib_converters,
)
fig = plt.figure()
ax = fig.add_subplot(111)
times = [datetime.datetime(2021, 1, 1)]
while len(times) < 7:
times.append(times[-1] + datetime.timedelta(seconds=120))
y_vals = np.arange(5)
time_axis, y_axis = np.meshgrid(times, y_vals)
shape = (len(y_vals) - 1, len(times) - 1)
z_data = np.arange(shape[0] * shape[1])
z_data.shape = shape
try:
register_matplotlib_converters()
im = ax.pcolormesh(time_axis, y_axis, z_data)
# make sure this does not raise!
fig.canvas.draw()
finally:
deregister_matplotlib_converters()
this might be silly, but can you double check which version of matplotlib and which branch you're testing against? |
|
@yacth, this is looking close. Suggest you rebase on master, that should fix your build error. This does need tests and documentation as well if you are up for it. It is hard to tell if it is working without those. |
I did a rebase, hope this will solve the failing tests. Sure I will investigate the tests. But first I have some questions about the |
|
@yacth I've not been following this PR closely. Did you spell out the locator/formatter issues? I can imagine they are a bit thorny as there is not a natural locator for the Power Scale. Perhaps best so see how it has been used in other contexts and copy. |
|
Hi @yacth - are you interested in continuing this work? If so, please mark the PR as ready for review and let us know if you need help with the rebase, we have instructions for that here. Thanks! |
PR Summary
As per the issue #20355 requirements, I added the
PowerScaleandPowerTransformandInvertedPowerTransformin thescale.pyfile following the architecture of the log norm.I am willing to discuss the
sef_default_locators_and_formattersfunction to set the right Locator and Formatter of the scale.I am waiting for a review on my code and improve it if needed.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).