Skip to content

Fix numerical overflow in v_from_i for some conditions #226

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

Merged
merged 7 commits into from
Jul 27, 2016

Conversation

wholmgren
Copy link
Member

Closes #225.

@wholmgren wholmgren added the bug label Jul 24, 2016
@wholmgren wholmgren added this to the 0.4.0 milestone Jul 24, 2016
@wholmgren
Copy link
Member Author

The new v_from_i code yields different numbers on windows than on mac/linux, or at least it does on the appveyor testing service. It would be great if someone running windows can try to debug this. I think you can just run the code pasted in #225 using the parameters printed there as well as the parameters in the test_v_from_i_bigger function (see files changed). No need to git checkout and install my branch.

@cwhanse
Copy link
Member

cwhanse commented Jul 26, 2016

I'll do it but I'll need some instructions, I clicked around unsuccesfully for awhile to see the error or test that failed.

@wholmgren
Copy link
Member Author

@cwhanse
Copy link
Member

cwhanse commented Jul 26, 2016

On Win64 the line
output = pvsystem.v_from_i(190, 1.065, 2.89, 0, 7.05196029e-08, 10.491262)

produces

54.304092877335222

which is different from the asserted test value by

0.00013404354376689298

At the intermediate steps:
logargW is 677.4553451926995

lambertwterm is 670.9466091769514+0j (type issue???)

Can you post the values from the code that finds the asserted value?

@wholmgren
Copy link
Member Author

Interesting that your system returns a much closer but still different result.

I reran this on my mac...

pvsystem.v_from_i(190, 1.065, 2.89, 0, 7.05196029e-08, 10.491262)

produces

54.303958833791455

and logargW is 677.4553451926995

lambertwterm is 670.9466555588004+0j

Scipy's lambertw always returns a complex number type, even if the complex part is 0. It probably would be better to drop the complex part at the lambertwterm = lambertw(argW) line than at the end.

@cwhanse
Copy link
Member

cwhanse commented Jul 27, 2016

After moving the .real I get the same values you posted.

@wholmgren
Copy link
Member Author

Hm. I am guessing that the difference is due to something about the precision of real numbers and complex numbers, and that is going to depend a user's platform and compiler. Or, for many of us, the version of Continuum's numpy/scipy packages.

I removed the scipy=0.16.0 version restriction on our appveyor test suite, and the test now passes. The version restriction was only present due to a problem with the matlab file reader in Continuum's scipy 0.16.1-0.17.

Thanks for your help, Cliff.

@wholmgren wholmgren merged commit 1f73af6 into pvlib:master Jul 27, 2016
@wholmgren wholmgren deleted the vfromioverflow branch July 27, 2016 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants