removed outdated parsing tool

[uvchik]

var=tools.Parse(Vars,Expect)

The call above causes the following error:

AttributeError: 'module' object has no attribute 'Parse'

My changes fixed #21 for me.

[bmu]

Thanks. I think the lines above (Vars and Expect) should also be deleted, but I'm not sure. @wholmgren?

[uvchik]

You are right, these lines are obsolete now. I missed them because they didn't cause an error.
I added this to my pull request.

[wholmgren]

I'm not sure what the appropriate scope is for this PR, so I'll suggest a few courses of action.

A. Merge this.
B. Update variable names in code and docs to be consistent with the rest of the module (and PEP8). Make a simple test. Also fix the reference to pvl_ and numerous typos in the docstring. Finally, merge.
C. Have a discussion about what this function is supposed to do. Its argument list suggests that it accounts for surface tilt and azimuth, which it does not do. I don't think that it can support those arguments without recalculating the diffuse components, at which point you might as will use something like irradiance.total_irrad (which needs help too).

[wholmgren]

Looks like pvlib matlab does not have anything like a globalinplane function and it's left to the user to do this calculation. I'm wondering if this is for the best given the assumptions that go into it.

[robwandrews]

I'd advocate for keeping this function, but cutting it down as it doesn't need the surface tilt inputs. This is a good general convenience function. @wholmgren , I'll let you make the call, but I'd say lets merge this to make it functional, and put B into milestone 0.2.

[wholmgren]

@Calama-Consulting that sounds good to me. @uvchik are you interested in making these changes?

[uvchik]

I pushed the changes and formatted the code according to the pep8 rules I know.

[wholmgren]

can you fix this reference? incidence is also misspelled above.

[wholmgren]

Did you use an automated PEP8 tool? It all looks good, although there are so many changes that I might have missed something.

I would prefer for the returned DataFrame column names to be consistent across all functions, but I'm ok deferring this decision until later.

I think I misread @Calama-Consulting's comment and agreed to something inadvertently. I would prefer for finish this off with a new test before merging.

[uvchik]

No, I had bad experience with an automated PEP8 tool. But I use IDEs (Spyder or Ninja-IDE) and they have integrated pep8 analyses similar to common spell checkers. It's annoying sometimes, but helps a lot ;-)

I don't know Sphinx very good, but the references should be fine now.

[uvchik]

Is it possible to merge these commits, that would help us in our feedinlib-project.

You can pull it to a new branch, if you don't want it in your master. Although I think it is better than the old version.

[wholmgren]

I know that we're at an early stage still, but I would prefer to get in the habit of requiring new tests with all PRs that change or add functionality. So, it would be ideal if you could add a test (or tests) to test_irradiance.py.

[uvchik]

Sorry, I don't know exactly what you want me to do. Do you want to test the functionality or the quality of the results?

I could add a function call, like the one below to the test_irradiance module but I really don't know how these test routines work.

def test_globalinplane():
    aoi = irradiance.aoi(40, 180, ephem_data['apparent_zenith'],
                         ephem_data['apparent_azimuth'])
    am = atmosphere.relativeairmass(ephem_data['apparent_zenith'])
    gr_sand = irradiance.grounddiffuse(40, ghi, surface_type='sand')
    diff_perez = irradiance.perez(
        40, 180, irrad_data['DHI'], irrad_data['DNI'], dni_et,
        ephem_data['apparent_zenith'], ephem_data['apparent_azimuth'], am)
    irradiance.globalinplane(
        AOI=aoi, DNI=irrad_data['DNI'], In_Plane_SkyDiffuse=diff_perez,
        GR=gr_sand)

The results look good but I got some negative beam radiation due to AOI greater 90 degree.
We could set all AOIs greater 90 degree to 90 degree but this is might be a new issue and a new discussion.

AOI[AOI > 90] = 90

[uvchik]

Sorry, I didn't meant to close it on purpose. Just pushed the wrong button.

[wholmgren]

The ideal test(s) would cover both, but I'm ok with just the test shown above at this point.

The negative values are a problem; thanks for pointing that out. Rather than operating on a slice of AOI, which would modify the input array, we can clip the Eb calculation. So I suggest changing line 436 to

Eb = pd.Series(DNI * np.cos(np.radians(AOI))).clip_lower(0)

The extra pd.Series is there to keep it working with all float inputs.

Both the negative values and the all float inputs are the sort of corner cases that are nice have tested in the test suite, but again, not necessary for now.

Just noticed that the docs describe these inputs as either float or DataFrame, but it should be float or Series. Also, AOI must be >=0 and <=180 is no longer needed.

Thanks for your patience making this better!

[uvchik]

1. I added the test_globalinplane() function to the test_irradiance module.
2. I corrected the docstring (DataFrame to Series). Or do we have to say pandas-Series?
3. I removed the range limit for aoi (AOI must be >=0 and <=180) from the docstring although I think that a valid aoi should be between 0° and 180°. An aoi of 45° gives the same result as an aoi of -45°. That makes obvious no sense but I think by now we should leave that in the responsibility of the user. Of course it is good to be sure that the aoi() function from pvlib doesn't return angles outside this range but this has nothing to do with the globalinplane() function. In further versions we could check the aoi and return a warning like "angles out of range".
4. I followed your suggestion and added the clip_lower(0) method to line 436 (now: 440). Now it works fine with angles between 90° and 180°. I added a short note to the docstring that describes this behaviour. Please check for mistakes.

[wholmgren]

This looks good to me.

Not a big deal at this point, but I did notice that AOI in the note section is in lowercase. Was that intentional? I also see that you're using lowercase names for your variables in your test code. I'm ok with changing the globalinplane parameter and DataFrame column names and capitalization if you prefer something else. We need a issue and possibly several PRs dedicated to coming up with consistent vocabulary, spelling, and capitalization of PV terms. Since that will take awhile, you have a chance to set one standard here.

I forgot to mention that you can add a line referencing this change and issue to the what's new file. Add yourself to the contributors list too. Probably a good idea to rebase your commits on the current master before doing so.

[uvchik]

I changed the variable names to upper case to be consistent within the module.

I would use existing conventions like PEP8 because this is easier for new contributors. For variable names I like the convention table of pylint: http://pylint-messages.wikidot.com/messages:c0103

Pylint is a program to check your code. It marks all lines where you broke the rules. You can integrate pylint in many IDEs or editors (http://docs.pylint.org/ide-integration) or use it as a standalone tool to check your modules.

If you do the rebase, I will add this issue to the what's new wiki. Thank you :-)

[wholmgren]

Go ahead and edit whats new, commit, and push. It might create a merge conflict, which either of us can fix. I prefer rebase for a situation like this since it keeps things a little cleaner but it's not a big deal.

You'd need to do something close to this (untested):

git remote add upstream https://github.com/pvlib/pvlib-python.git
git fetch upstream
git rebase upstream/master

Then edit what's new, commit, and push. You may need to add -f to your git push command. No idea how to do it with a GUI.

Maybe you'd like to contribute to #37.

[uvchik]

I did the rebase.

[wholmgren]

Great, go ahead an make your changes to whats new and then I'll merge.

[uvchik]

Done.

[wholmgren]

I don't see the changes to the whats new file. Maybe a missed commit?

[uvchik]

I don't have the right to write the wiki, so it ended up as another pull request #38.

[wholmgren]

Oh, you're just editing another source file in the repo and it should be included in this PR. The only difference is that it's source code for sphinx instead of python. Just merge your patch-1 branch into your master branch.

The wiki is here and you should be able to edit that if you're so inclined.

[uvchik]

Okay, I got it. Sorry, I was a little confused.

[wholmgren]

Great! Thank you for the contribution and for your patience!

[uvchik]

Thanks, it was interesting for me, too.