-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Bug within scaling.py wavelet calculation methodology #1206
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
Comments
@jranalli thanks for finding and reporting this. Can I ask how you contacted PVLIB MATLAB? Because I maintain that repository and I didn't see the email, so we need to fix something on our end with communications. |
@cwhanse Now that I look again, I think I used the wrong form. It was just a general Questions and Comments link for the PV Performance Modeling Collaborative at the bottom of the page. I didn't see any contact point for the PV_LIB MATLAB library and I also didn't know about the github repo for it, but now I do! I do have a fix for the MATLAB code as well, but I don't see that part of the library on github. If you'd like me to open an issue on that repository as well, I'd be happy to do so, but if there's some other pathway or contact point since that's kind of listed as a separate package of the code, please let me know. Either way, do you think it's appropriate to fix this, or does there need to be a conversation with the original author of that MATLAB code? If everything is fine to go ahead with it here, I can just put together my fix as a pull request for review. |
And did my own looking: pvl_WVM is in it's own Matlab archive, separate from PVLIB for Matlab. The WVM code is only available as a download from pvpmc.sandia.gov, whereas PVLIB for Matlab is on github. I've sent the bug report to Matt Lave, the originator of the WVM algorithm and code. We'll likely welcome the bug fix but I'd like to hear Matt's view first. |
OK sounds good. If he or you want to connect for more detail on the issue, you can get contact info for me at my Faculty Page. |
for the record: bug is confirmed via separate communication with the WVM algorithm author. |
Describe the bug
Mathematical error within the wavelet computation for the scaling.py WVM implementation. Error arises from the methodology, as opposed to just a software bug.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
For a discrete wavelet transform (DWT) the sum of all wavelet modes should equate to the original data.
Versions:
pvlib.__version__
: 0.7.2pandas.__version__
: 1.2.3Additional context
This bug is also present in the PV_LIB Matlab version that was used as the basis for this code (I did reach out to them using the PVLIB MATLAB email form, but don't know who actually wrote that code). Essentially, the existing code throws away the highest level of Detail Coefficient in the transform and keeps an extra level of Approximation coefficient. The impact on the calculation is small, but leads to an incorrect DWT and reconstruction. I have a fix that makes the code pass the theoretical test about the DWT proposed under 'To Reproduce' but there may be some question as to whether this should be corrected or left alone to match the MATLAB code it was based on.
The text was updated successfully, but these errors were encountered: