CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:
http://www.ral.ucar.edu/projects/summa
GNU General Public License v3.0
80 stars 105 forks source link

Summa-Actors compatibility update #564

Closed KyleKlenk closed 5 months ago

KyleKlenk commented 5 months ago

Adjusted where fracJulDay, yearLength, and tmZoneOffsetFracDay are imported from globalData. I also moved them as arguments to the subroutines they were used in.

This is because our Summa-Actors project (https://git.cs.usask.ca/numerical_simulations_lab/actors/Summa-Actors) has these variables local to the "HRU_Actor" so it can be computed asynchronously. These changes allow Summa-Actors to import the majority of these existing files without modification or keeping its own copy. This way any changes made in this repo are automatically reflected in Summa-Actors.

The changes requested here do not change any functionality and were tested with the following laugh-tests:

The changes made in the pull request produced the exact results from the current main branch.

Additionally changed the cpu_time function call to system_clock. This is because system_clock is thread safe. When multithreading (Summa-Actors) cpu_time() does not produce correct results for each HRU where system_clock does.

wknoben commented 5 months ago

Looks good to me.

I'll leave it open for a day or so to give @andywood a chance to chime in, but I don't see anything here that would affect the operational feasibility of running SUMMA. @KyleKlenk Presumably any of the Actor-specific stuff doesn't affect runtimes without Actors, right?

andywood commented 5 months ago

Thanks @wknoben, I had taken a quick look and saw some evidence of effort to make it backward compatible for the non-Agents use case. The other check would be for unusual added dependencies (eg non-standard libraries) that would be required to compile summa even if one were not using agents. I didn't check for that. -Andy

On Sun, Apr 21, 2024 at 12:58 PM Wouter Knoben @.***> wrote:

Looks good to me.

I'll leave it open for a day or so to give @andywood https://github.com/andywood a chance to chime in, but I don't see anything here that would affect the operational feasibility of running SUMMA. @KyleKlenk https://github.com/KyleKlenk Presumably any of the Actor-specific stuff doesn't affect runtimes without Actors, right?

— Reply to this email directly, view it on GitHub https://github.com/CH-Earth/summa/pull/564#issuecomment-2068002105, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARK2ZIUB33N3FV446ATY6OLWLAVCNFSM6AAAAABGP5WZUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRYGAYDEMJQGU . You are receiving this because you were mentioned.Message ID: @.***>

wknoben commented 5 months ago

@andywood As far as I can tell system_clock is a fortran built-in. Maybe @KyleKlenk can confirm that this is the case?

KyleKlenk commented 5 months ago

system_clock is a Fortran intrinsic and is part of the Fortran standard. The changes as part of this pull request should have not introduced anything extra that users would need to install or modify. The main branch after this pull request is intended to work without actors, in the way users would already expect.

For clarity, here is some output of the Celia Laugh Test wallClockTime variable using cpu_time on the left and system_clock on the right.

cpu_time system_clock
0.090014 0.00740458304062486
0.00145499999999998 0.00145566405262798
0.00122899999999992 0.0012408560141921
0.00113300000000005 0.00113162596244365
wknoben commented 5 months ago

Alright, sounds good to me, I think this can be merged. @andywood any further comments?

wknoben commented 5 months ago

I guess this is good to go, thanks Kyle!