Safely calculate dni from dhi and ghi #250

uvchik · 2016-10-18T16:36:19Z

This PR is a little bit confusing, because it works on top of PR #239. The changes of this PR start with commit 6907b6c. It will be easier to read once PR #239 is merged.

The aim of this PR is to safely calculate DNI from GHI and DHI. There are different approaches and we may collect them within this function. A method parameter (or something similar) can be used to switch between the different approaches.

As this PR is a spin-off from PR #239 there are already two comments regarding this PR.

@adriesse wrote:

For your specific example, you can keep DNI values somewhat sane by not allowing them to be greater than a value obtained from a clear-sky model. Calculating the sun angles for the midpoint of the averaging interval is common practice (I hope). For long time steps like one hour, you can split the intervals that includes sunrise/set into two parts (one before and one after the event).

@cwhanse wrote:

Ø Calculating the angles for the midpoint of the averaging interval is common practice (I hope). It ought to be common practice. SAM does it, as does PVsyst I think, but other applications? We ought to build this capability into pvlib I think, as part of model chain – a kwarg switch perhaps?

A rough approach is to use the horizontal value for low angles, as there might not be relevant irradiation at small angles (high zenith angles) or set it to zero.

dni = (ghi - dhi) / tools.cosd(zenith)
dni[zenith > 88] = ghi[zenith > 88] - dhi[zenith > 88]
# or
dni[zenith > 88] = 0

What methods do we want to implement?
How do we want to switch between methods?

@birgits will help to implement this feature.

wholmgren · 2016-12-02T17:38:20Z

sorry for overlooking this @uvchik. I'd suggest some kind of filtering for the simple calculation but you could convince me that it's the user's responsibility. The comments above suggested to me that you were thinking of doing something more for time averages and sunrise/sunset transitions. Is that the case?

birgits · 2016-12-17T18:55:09Z

Sorry for having neglected this PR for so long. I have been working on it and am still having some problems, but I will post what I've got so far at the beginning of next week.

The comments above suggested to me that you were thinking of doing something more for time averages and sunrise/sunset transitions.

This is right. When calculating the DNI from GHI and DHI it may happen that for zenith angles close to 90°, so sunrise/sunset, the calculated DNI is unreasonable high or negative. The method we want to implement will "correct" those wrong values using different approaches.

birgits · 2016-12-20T16:13:48Z

This is a first suggestion for how to correct calculated DNI values that are unreasonably high or even negative in sunrise/sunset transitions. For the dataset I used the two methods implemented right now give nearly the same results.
@uvchik and I decided that it would be good to let the user know for how many timesteps the DNI had to be corrected, maybe serving as an indication on how good the quality of the given weather dataset is. We also found out that if corrections were only conducted for sunrise or sunset hours, a likely reason is that the timezone is not correct, and decided to also give out a warning in that case.

adriesse · 2016-12-21T08:38:43Z

I am afraid that you could see problematic DNI values well before 88 degrees with real measurements due to cosine and alignment errors. Maybe you could make a gradual constraint that goes from the clear sky value at 80 degrees and zero at 90 degrees? In real data you may also need to constrain the DNI to non-negative values.

birgits · 2016-12-21T08:59:58Z

Thank you @adriesse for your comments and suggestions, I will try to implement them!
I was also wondering, since you suggested to take the clearsky DNI as an upper bound to get "sane" values during sunrise/sunset transitions, if the clearsky DNI should always be higher than the calculated or measured DNI, as is the case for GHI? I couldn't really find anything on that.

adriesse · 2016-12-21T09:53:11Z

Since the clear sky DNI you use here is from a model, it is going to represent some kind of average conditions, so certainly the true DNI could be a bit higher than the model values. I suppose you could even add 10% to the model value and still achieve your objective of clipping unrealistic values.

A separate comment: in some cases it might be preferable to replace suspicious values with nan's.

birgits · 2016-12-21T10:02:11Z

A separate comment: in some cases it might be preferable to replace suspicious values with nan's.

I could add a parameter that can be used to specify if values should be corrected or set to nan. What do you think?

About setting the clearsky DNI as upper bound - to me it seems a bit random to add 10% to the modelled clearsky value, but maybe this could also be an optional parameter?

adriesse · 2016-12-21T10:17:15Z

I think an option for nan's would be nice. I have no opinion about the 10%.

wholmgren · 2016-12-21T15:47:50Z

Thanks @birgits. I agree with @adriesse's comments. A couple of implementation things:

The function signature should use basic inputs such as clearsky_dni and zenith rather than a Location. This gives users more more flexibility, keeps the function as simple as possible, and makes it faster. I'm guessing that most users will already have that data available in their script or interpreter session, too.

I'd also prefer that we not add the timezone speculation code. Generally speaking, users have a responsibility to get their timezones right. However, we could consider a more robust, dedicated function for checking timezones in a separate issue/pr (there was also some talk of this in one of the IO package/module threads).

It might help if you (and others) provide some examples of some real data that you're trying to clean up.

birgits · 2016-12-22T13:35:59Z

The function signature should use basic inputs such as clearsky_dni and zenith rather than a Location.

I agree with you. I just thought it would be easier for the user to only have to create a location object and not have to worry about how to get the zenith and which clearsky model to use, but have the definition take care of that. It took me quite a while anyway to go through all the clearsky models and try to understand how I can use them and what inputs I needed and I was very happy when I found that the location object does all that for me already. But you are right, it will be more straight forward the other way.

birgits · 2017-04-07T12:35:45Z

I took out the different methods and instead added two new parameters for the zenith angles between which the unreasonable values are set to NaN or corrected, as suggested by @wholmgren and @uvchik. What do you think?

A few things I want to point out:

I chose the default values for lower_cutoff_zenith and upper_cutoff_zenith from an early comment by @adriesse. They are open for discussion.
Negative DNI values as well as nonzero values above the upper_cutoff_zenith (above 88 for example) are set to NaN. Before there was also the option to set them to zero.
In the modelchain the function irradiance.dni is now by default called with the calculated clearsky_dni.

uvchik · 2017-04-10T09:48:53Z

I like the dni-function.
It might be controversial but I would remove the **kwargs attribute from both functions (dni and complete_irradiance) and change the following lines in complete_irradiance:

-                clearsky_dni=kwargs.get('clearsky_dni',
-                                        self.location.get_clearsky(times).dni),
-                clearsky_tolerance=kwargs.get('clearsky_tolerance', 1))
+                clearsky_dni=self.location.get_clearsky(times).dni),
+                clearsky_tolerance=1))

People should use this function or do their own stuff.

A NaN to Zero is not important for me. Anyway we might want to add it to the ModelChain but not to the dni function.
You have to rebase the actual pvlib-dev branch on top of yours to solve the conflict.

birgits · 2017-05-07T19:06:53Z

Thank you @uvchik for your comments. @wholmgren and @adriesse do you have any more comments or suggestions?

The only conflict was that both me and the upstream master added a test at the end of a test file so the solution was easy: just take both.

I had a mishap which removed these files from being version controlled. This should fix it.

Again, the only conflict was that both me and the upstream master added a test at the end of a test file Solution: just take both. I have no idea, what happened during the first merges. Everything should be fine now.

wholmgren · 2017-05-10T17:11:21Z

I am generally ok with these additions. I'll make minor comments later this week.

wholmgren

here are the minor comments

wholmgren · 2017-05-12T22:12:32Z

pvlib/irradiance.py

+        Default: 88.
+
+    lower_cutoff_zenith : float
+        Zenith angle above which DNI values greater the clearsky DNI (plus


"plus" should be "times"

wholmgren · 2017-05-12T22:15:07Z

pvlib/irradiance.py

+    """
+
+    # calculate DNI
+    dni_tmp = (ghi - dhi) / tools.cosd(zenith)


why do you need dni_tmp and dni? maybe a holdover from a previous implementation?

wholmgren · 2017-05-12T22:18:51Z

pvlib/irradiance.py

+    # upper_cutoff_zenith that are greater than the clearsky tolerance
+    if clearsky_dni is not None:
+        dni[(zenith >= lower_cutoff_zenith) & (zenith <= upper_cutoff_zenith) &
+            (dni > (clearsky_dni * clearsky_tolerance))] = (clearsky_dni *


You're doing the same calculation twice, so it may be slightly faster and slightly more readable if you define and use max_dni = clearsky_dni * clearsky_tolerance.

wholmgren · 2017-05-12T22:20:14Z

pvlib/irradiance.py

+    dni[dni < 0] = float('nan')
+
+    # set non-zero DNI values for zenith angles >= upper_cutoff_zenith to NaN
+    dni[(zenith >= upper_cutoff_zenith) & (dni != 0)] = float('nan')


The doc string suggested to me that the comparison should be strictly greater than, rather than greater than or equal to.

Would that work and be correct English?

Zenith angle from which on nonzero DNI values are set to NaN.

Zenith angles greater than or equal to upper_cutoff_zenith will be set to NaN.

wholmgren · 2017-05-12T22:20:30Z

pvlib/irradiance.py

+    # correct DNI values for zenith angles between lower_cutoff_zenith and
+    # upper_cutoff_zenith that are greater than the clearsky tolerance
+    if clearsky_dni is not None:
+        dni[(zenith >= lower_cutoff_zenith) & (zenith <= upper_cutoff_zenith) &


same as above regarding comparison type

wholmgren · 2017-05-12T22:22:21Z

pvlib/irradiance.py

@@ -2048,3 +2048,70 @@ def _get_dirint_coeffs():
        [0.743440, 0.592190, 0.603060, 0.316930, 0.794390]]

    return coeffs[1:, 1:, :, :]
+
+
+def dni(ghi, dhi, zenith, clearsky_dni=None, clearsky_tolerance=1,


Would we be better off with a more descriptive name? dni_from_components? I'm not sure.

I find the name dni_from_components a bit confusing since components could mean different things. Maybe we just name it after what it does dni_from_ghi_and_dhi?

According to aoi we could keep dni, but I like get_dnior calculate_dni definitely better than dni_from_something. We haven't used it so far and in my opinion the parameters should be part of the API documentation and not of the name.

Seems that dni is moderately acceptable to all.

wholmgren · 2017-05-12T22:22:55Z

pvlib/irradiance.py

+
+
+def dni(ghi, dhi, zenith, clearsky_dni=None, clearsky_tolerance=1,
+        upper_cutoff_zenith=88.0, lower_cutoff_zenith=80.0, **kwargs):


no kwargs here

wholmgren · 2017-05-12T22:23:12Z

pvlib/irradiance.py

+    -------
+    dni : Series
+        The modeled direct normal irradiance.
+


no extra space needed

wholmgren · 2017-05-12T22:23:37Z

pvlib/modelchain.py

@@ -623,7 +623,7 @@ def effective_irradiance_model(self):
            fd*self.total_irrad['poa_diffuse'])
        return self

-    def complete_irradiance(self, times=None, weather=None):
+    def complete_irradiance(self, times=None, weather=None, **kwargs):


as discussed elsewhere, probably no kwargs here, too.

Done.
I had to set a default value for the clearsky_tolerance and chose 1.1, again according to an early comment from @adriesse. Would you suggest to have the clearsky_tolerance as an input into the complete_irradiance function, so it can be changed by the user or should the user write his/her own function in case he/she wants to change those parameters?

adriesse · 2017-05-15T09:22:24Z

Good work. It occurs to me that the upper and lower cutoff names might not be entirely intuitive. How about: zenith_threshold_for_clearsky_limit, zenith_threshold_for_nan or something like that?

birgits · 2017-05-15T17:52:50Z

Sounds good @adriesse! How about zenith_threshold_for_zero_dni for the upper_cutoff_zenith, since the DNI for zenith angels above that value should be zero?

birgits · 2017-05-15T18:10:18Z

And I don't know what to do about the failing tests... All tests pass on my computer. Does anyone know what's going wrong?

wholmgren · 2017-05-15T18:13:00Z

see #326 regarding the failing tests. I will merge the fix and retrigger the build.

…d comments lower_cutoff_zenith is renamed to zenith_threshold_for_clearsky_limit upper_cutoff_zenith is renamed to zenith_threshold_for_zero_dni

birgits · 2017-05-24T08:24:11Z

Thank you for fixing the failing tests @wholmgren! Do you think the PR is ready to be merged?

wholmgren · 2017-05-25T14:21:04Z

Looks good to me. Can you add a note and your name to the 0.4.5 whats new file?

wholmgren · 2017-05-29T15:05:49Z

thanks @birgits and @uvchik!

This was referenced Oct 18, 2016

Safely calculate dni from dhi and ghi. #249

Closed

Making the irradiation and weather DataFrame of the ModelChain more flexible #239

Merged

wholmgren added the enhancement label Oct 21, 2016

wholmgren added this to the 0.4.2 milestone Oct 21, 2016

Uwe Krien and others added 4 commits November 14, 2016 09:17

creating basic function to calculate DNI from GHI and DHI

7d54462

replace equation with new function for test purposes

b5994c6

change types to satisfy type checks

abf02af

remove warning if new function is used

b9f6365

uvchik force-pushed the master branch from 44d6958 to b9f6365 Compare November 14, 2016 08:19

uvchik force-pushed the master branch 2 times, most recently from e3eca71 to b9f6365 Compare November 24, 2016 13:42

wholmgren modified the milestones: 0.4.3, 0.4.2 Dec 2, 2016

Birgit Schachler added 5 commits December 20, 2016 16:54

adapt input and output of dni method

89fea56

adapt docstring to new inputs

21f5c5f

add clearsky method to correct calculated dni

a2b6413

add cutoff method to correct calculated dni

34865e5

add logging warning and debug messages

8fd63b8

Birgit Schachler added 2 commits December 21, 2016 10:41

increase code quality

a46cdfa

remove non-ascii characters

98b6fe7

birgits added 3 commits May 10, 2017 17:26

Merge upstream changes on master

f3bd094

The only conflict was that both me and the upstream master added a test at the end of a test file so the solution was easy: just take both.

Add accidentally un-added files

e1f5f58

I had a mishap which removed these files from being version controlled. This should fix it.

Merge upstream changes on master (for real now)

9949661

Again, the only conflict was that both me and the upstream master added a test at the end of a test file Solution: just take both. I have no idea, what happened during the first merges. Everything should be fine now.

wholmgren reviewed May 12, 2017

View reviewed changes

Birgit Schachler added 7 commits May 15, 2017 10:38

Remove kwargs from dni function

4398017

Remove unnecessary dni_tmp

aa82e57

Remove blank line at end of docstring

0f5f113

Add max_dni to do calculation only once

ac98688

Correct comment

60a922c

Correct docstring for lower_cutoff_zenith

8c05db1

Remove kwargs from complete_irradiance

2f08557

Birgit Schachler added 3 commits May 17, 2017 13:45

Correct docstrings

9df766f

Use same default value for clearsky_tolerance as in ModelChain

e7a43df

Change names of lower_ and upper_cutoff_zenith and adapt docstring an…

d07ab49

…d comments lower_cutoff_zenith is renamed to zenith_threshold_for_clearsky_limit upper_cutoff_zenith is renamed to zenith_threshold_for_zero_dni

birgits added 2 commits May 29, 2017 11:03

Merge remote-tracking branch 'upstream/master'

2c253cb

add safely calculate dni from dhi and ghi (pvlib#250)

e410d6c

wholmgren merged commit f8423c9 into pvlib:master May 29, 2017

adriesse mentioned this pull request Feb 20, 2018

Dividing by cos(solar_zenith) in haydavies and reindl functions #432

Closed

wholmgren mentioned this pull request May 14, 2018

change permissions on irradiance.py #460

Closed



		def dni(ghi, dhi, zenith, clearsky_dni=None, clearsky_tolerance=1,
		upper_cutoff_zenith=88.0, lower_cutoff_zenith=80.0, **kwargs):

Safely calculate dni from dhi and ghi #250

Safely calculate dni from dhi and ghi #250

Conversation

uvchik commented Oct 18, 2016

wholmgren commented Dec 2, 2016

birgits commented Dec 17, 2016

birgits commented Dec 20, 2016

adriesse commented Dec 21, 2016

birgits commented Dec 21, 2016

adriesse commented Dec 21, 2016

birgits commented Dec 21, 2016

adriesse commented Dec 21, 2016

wholmgren commented Dec 21, 2016

birgits commented Dec 22, 2016

birgits commented Apr 7, 2017

uvchik commented Apr 10, 2017

birgits commented May 7, 2017

wholmgren commented May 10, 2017

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriesse commented May 15, 2017

birgits commented May 15, 2017

birgits commented May 15, 2017

wholmgren commented May 15, 2017

birgits commented May 24, 2017 • edited Loading

wholmgren commented May 25, 2017

wholmgren commented May 29, 2017

birgits commented May 24, 2017 •

edited

Loading