Should infinite_sheds.get_irradiance be updated to return shaded fraction?

[williamhobbs]

There was discussion here #1689 and here #1725 about having a public shaded_fraction function.

That seems like it would eventually be very helpful, but in the meantime, would it make sense to update infinite_sheds.get_irradiance to return the shaded fraction values that are already being calculated? This would make it easier for users to add their own shade loss calculations for both fixed and tracking systems.

If this makes sense, I can submit an issue and PR, but I thought a discussion first would be best. Tagging @mikofski @AdamRJensen @cwhanse @kandersolar since we have all discussed related items (apologies if I'm forgetting someone...).

[cwhanse]

Are you proposing to make pvlib.bifacial.infinite_sheds._shaded_fraction public? I support that.

I'm not sure how I feel about enlarging the purpose of the two get_irradiance functions. They are already returning large DataFrames so more columns isn't a concern. What other intermediate, non-irradiance quantities would also be returned?

[williamhobbs]

I'm proposing changing

pvlib-python/pvlib/bifacial/infinite_sheds.py

Lines 369 to 372 in fc8eebb

	output = {
	'poa_global': poa_global, 'poa_direct': poa_direct,
	'poa_diffuse': poa_diffuse, 'poa_ground_diffuse': poa_gnd_pv,
	'poa_sky_diffuse': poa_sky_pv}

to

output = {
        'poa_global': poa_global, 'poa_direct': poa_direct,
        'poa_diffuse': poa_diffuse, 'poa_ground_diffuse': poa_gnd_pv,
        'poa_sky_diffuse': poa_sky_pv, 'shaded_fraction': f_x}

and

pvlib-python/pvlib/bifacial/infinite_sheds.py

Lines 542 to 555 in fc8eebb

	colmap_front = {
	'poa_global': 'poa_front',
	'poa_direct': 'poa_front_direct',
	'poa_diffuse': 'poa_front_diffuse',
	'poa_sky_diffuse': 'poa_front_sky_diffuse',
	'poa_ground_diffuse': 'poa_front_ground_diffuse',
	}
	colmap_back = {
	'poa_global': 'poa_back',
	'poa_direct': 'poa_back_direct',
	'poa_diffuse': 'poa_back_diffuse',
	'poa_sky_diffuse': 'poa_back_sky_diffuse',
	'poa_ground_diffuse': 'poa_back_ground_diffuse',
	}

to

    colmap_front = {
        'poa_global': 'poa_front',
        'poa_direct': 'poa_front_direct',
        'poa_diffuse': 'poa_front_diffuse',
        'poa_sky_diffuse': 'poa_front_sky_diffuse',
        'poa_ground_diffuse': 'poa_front_ground_diffuse',
        'shaded_fraction': 'shaded_fraction_front',
    }
    colmap_back = {
        'poa_global': 'poa_back',
        'poa_direct': 'poa_back_direct',
        'poa_diffuse': 'poa_back_diffuse',
        'poa_sky_diffuse': 'poa_back_sky_diffuse',
        'poa_ground_diffuse': 'poa_back_ground_diffuse',
        'shaded_fraction': 'shaded_fraction_back',
    }

But maybe making pvlib.bifacial.infinite_sheds._shaded_fraction public makes more sense? The fact that it is limited to zero cross axis slope and a few other limitations mentioned in #1689 and #1725 are maybe reasons not to do that?

[kandersolar]

I think I'm +1 to including shaded fraction values in the return values from get_irradiance[_poa].

Without thinking too hard about it, I'm not seeing any other quantities that would be useful to return as well. Or at least not any that are of primary interest for PV modeling.

For a general-purpose public function, I suggest we hold off until the work mentioned in #1869 (comment) is available to use.

[williamhobbs]

Would the output variable names I used above (shaded_fraction, shaded_fraction_front, and shaded_fraction_back) be appropriate? It's currently just f_x, but that doesn't seem descriptive enough for a returned value.

[AdamRJensen]

I know @kandersolar is working on a general shade method for calculating shading/backtracking on uneven terrain that could fulfill all of the use cases described in PR 1689 and 1725.

Kevin can correct me if I am wrong, but if his new formulation can do it all, then I think we should holdoff on adding new specific functions or making existing functions public.

[williamhobbs]

I think what I am proposing is not a new function, or making an existing function public, it's just returning values that are already being calculated. Maybe that's the same as "making it public"?

See the example code changes in my reply to Cliff above, #1869 (reply in thread).

[williamhobbs]

I've submitted an issue #1870 and PR #1871.