search

srlabUsask / crhmcode

GNU General Public License v3.0
8 stars 4 forks source link

Problem with results from release 1.1 #10

Closed KevinShook closed 3 years ago

KevinShook commented 3 years ago

I was able to run release 1.1. The execution time was excellent - about 1/3 of the old Windows version. The executable file is also very small, which is great to see Unfortunately, there are very large errors in some of the values. I had noticed similar issues in release 1.0, as well.

As you can see from the figure, the annual depth of annual discharge is orders of magnitude too large in the crhmcode release 1.1 model. This would appear to be because the actual evaporation depths are too small, as the depths of precipitation are identical. windows_crhmcode_annual_flux_depths This model uses 2 different methods for calculating evaporation. Penman-Monteith is used by HRUs 2-4, while HRU 11 uses Priestley-Taylor. Both are giving incorrect results. I checked the snow accumulation from the module PBSM, and it appears to be giving the same results in both models. windows_crhmcode_annual_peak_swe

Therefore, the issue appears to be either in the module evap_Resist or within one of the modules that it uses.

jhs507 commented 3 years ago

Alright I was unsure if there was a valid reason to change it based on the previous time step.

KevinShook commented 3 years ago

Unfortunately CRHM doesn't do a good job of indicating which parameters can and cannot change.

jhs507 commented 3 years ago

Kevin Schneider pointed out to me that in the pbsm module there is a check if the SWE is greater than zero and if it is then inhibit_evap is set.

This is present in both borland and gcc code.

Does this make sense in terms of the physics?

for (hh = 0; chkStruct(); ++hh) { // snow cover inhibits evaporation

      if(SWE[hh] > 0.0){
        const_cast<long*> (inhibit_evap)[hh] = 1;
        snowdepth[hh] = Common::DepthofSnow(SWE[hh]);
      }
      else{
        const_cast<long*> (inhibit_evap)[hh] = 0;
        snowdepth[hh] = 0.0;
      }
    }
KevinShook commented 3 years ago

That's weird. I'll have to check. K

Dr. Kevin Shook, P.Eng.

Research Scientist

Centre for Hydrology

Department of Geography and Planning

University of Saskatchewan

101.07 - 121 Research Drive

Saskatoon, SK

Canada

S7N 1K2

(306)966-5514

From: jhs507 notifications@github.com Sent: February 22, 2021 3:44 PM To: srlabUsask/crhmcode crhmcode@noreply.github.com Cc: Shook, Kevin krs350@mail.usask.ca; Author author@noreply.github.com Subject: Re: [srlabUsask/crhmcode] Problem with results from release 1.1 (#10)

CAUTION: External to USask. Verify sender and use caution with links and attachments. Forward suspicious emails to phishing@usask.ca

Kevin Schneider pointed out to me that in the pbsm module there is a check if the SWE is greater than zero and if it is then inhibit_evap is set.

This is present in both borland and gcc code.

Does this make sense in terms of the physics?

for (hh = 0; chkStruct(); ++hh) { // snow cover inhibits evaporation

  if(SWE[hh] > 0.0){
    const_cast<long*> (inhibit_evap)[hh] = 1;
    snowdepth[hh] = Common::DepthofSnow(SWE[hh]);
  }
  else{
    const_cast<long*> (inhibit_evap)[hh] = 0;
    snowdepth[hh] = 0.0;
  }
}

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/srlabUsask/crhmcode/issues/10#issuecomment-783695262, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACUJ4TP7PGKQWHRGBZQA5F3TALF5XANCNFSM4XMCVWEA.

KevinShook commented 3 years ago

I checked with Logan. Wow - so it is intended to be changed internally, but it can also be specified. So, I guess we keep the code where it suppresses evap when there's a snowcover. It makes sense because CRHM is doing snow sublimation instead of evaporation. However, it's dangerous to have it as a user-set parameter as well. I guess we need to see if it's being changed by rainfall, too.

KevinShook commented 3 years ago

I wonder if the issue is that it thinks rainfall is snowfall. All of the examples I found in the new code where evap = 0, when precip > 0 were in the summer.

jhs507 commented 3 years ago

Okay I will see if I can find something in that area.

KevinShook commented 3 years ago

I think that I have solved the problem. It is probably caused by a) crhmcode using double precision and b) the existence of a bad threshold value. As we discussed, if there is any snow, then evaporation is set to zero. In the example I was looking on May 26, 1972, a tiny (round off) amount of the precipitation was falling as snow: image

This is what is causing the evaporation to be suppressed. I will contact Logan and John about this. A concern is that this is also happening for the Borland CRHM for other dates, and this is probably not correct

jhs507 commented 3 years ago

Ah so if the tiny amount of snowfall turns off the evaporation that sounds like our problem. The obvious temporary solution is to treat such a small amount of snowfall as no snowfall. Good catch.

jhs507 commented 3 years ago

I changed the threshold for the pdsm module to turn off evaporation and doing so has reduced the total amount of steps that evaporation is different between borland and crhmcode versions by an order of magnitude.

I will test to see if this has a similar effect on your desired final outputs.

I think we will have to employ a more intelligent fix but this was definitely the cause of these issues. However my current threshold of SWE of 0.0000001 to be considered zero for purposes of turning off evaporation causes issues in the other direction. ie turning on evaporation when it was previously off.

KevinShook commented 3 years ago

Thanks. I think that this may well need a bigger fix. Logan Fang is running the old Borland project to see if he gets the same results.

jhs507 commented 3 years ago

There are also many other modules that turn off evaporation that will need to be changed to accommodate this pattern. Hopefully that helps bring some other results that were out of sync in to alignment as well.

jhs507 commented 3 years ago

Here are what your outputs look like after 2 years with this change being made. These results are about as close to matching the borland version that we have come so far.

after_pbsm_change_comparison.xlsx data.zip

KevinShook commented 3 years ago

Thanks very much - good to see. We're going to have to fix the existing Borland CRHM first, and will then add it to crhmcode.

jhs507 commented 3 years ago

Alright, now the greatest difference seems to be with the soil moisture. I will see if I can find anything within that calculation that could be having similar issues caused by moving to double precision.

KevinShook commented 3 years ago

As soil_moisture is a state variable, it is affected by all fluxes, including evaporation. It also affects the actual evaporation, which is reduced as the soil dries out. It's possible that some of the other fluxes have zero thresholds as well

KevinShook commented 3 years ago

I talked to John Pomeroy and he agrees that we should be checking the SWE against a threshold which is specified as a parameter. I'll upload the new code when it's finished.

jhs507 commented 3 years ago

That makes sense. Do you intend for this parameter to be specific to the SWE turning off evap or a general threshold for comparisons with zero?

On Tue, Feb 23, 2021 at 19:14 KevinShook notifications@github.com wrote:

I talked to John Pomeroy and he agrees that we should be checking the SWE against a threshold which is specified as a parameter. I'll upload the new code when it's finished.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/srlabUsask/crhmcode/issues/10#issuecomment-784657549, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFQPCKIDFIZPYA3OPN4NY4DTARHG5ANCNFSM4XMCVWEA .

KevinShook commented 3 years ago

It will just be for the SWE as that has physical meaning - plants can still transpire water when there's just a little bit of summer snow on them. However, the comparisons with zero thresholds are certainly something that we have to check on.

jhs507 commented 3 years ago

Who will be making these changes? I would like to get the source code from them if at all possible.

KevinShook commented 3 years ago

The changes are being made by Logan Fang, whom is maintaining the old code. I'll make sure that you get it when it's ready.

KevinShook commented 3 years ago

Here's the revised source code. It seems to generate much more reasonable results. Source_022421.zip

jhs507 commented 3 years ago

Thank you, I have begun adapting the crhmcode to accommodate this change.

Has Logan uploaded a new executable?

KevinShook commented 3 years ago

He has. It's accessible through our website https://research-groups.usask.ca/hydrology/modelling/crhm.php Or directly thru dropbox https://www.dropbox.com/s/19o5oqpw49ko4ia/CRHM_Distribution.zip?dl=0

jhs507 commented 3 years ago

Thank you.

jhs507 commented 3 years ago

What value are you using for the newly declared parameter?

KevinShook commented 3 years ago

I ran it with a value of 5 mm for all HRUs

jhs507 commented 3 years ago

I ran a two year run with the updated code. It seems that we are now quite close. Do you feel these results are what you expect?

after_pbsm_change.xlsx data.zip

KevinShook commented 3 years ago

That's excellent to see, and very good progress. Well done. I'm just checking on some issues with the net solar radiation. I'll create a new issue for it after I've double-checked with the new code