Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

albgrd and albgri history fields depend on decomposition, for urban points #17

Open
billsacks opened this issue Dec 16, 2017 · 6 comments · May be fixed by #2893
Open

albgrd and albgri history fields depend on decomposition, for urban points #17

billsacks opened this issue Dec 16, 2017 · 6 comments · May be fixed by #2893
Labels
bfb bit-for-bit blocked: dependency Wait to work on this until dependency is resolved bug something is working incorrectly good first issue simple; good for first-time contributors priority: low Background task that doesn't need to be done right away.

Comments

@billsacks
Copy link
Member

Bill Sacks < sacks@ucar.edu > - 2013-08-31 07:26:34 -0600
Bugzilla Id: 1806
Bugzilla CC: muszala@ucar.edu, oleson@ucar.edu, rfisher@ucar.edu,

I believe that this problem only affects history fields, and doesn't actually affect the operation of the model. This is a restatement of the initial bug in bug 1310 (most of the comments in that bug report really relate to a different bug).

For urban points, albgrd and albgri depend on the decomposition - either the number of tasks or the number of threads per task.

I believe that what is going on is the following (copied from bug 1310; I haven't checked carefully whether the behavior has changed slightly from this):

(1) In UrbanMod.F90: UrbanAlbedo: A count is made of the number of urban
landunits with coszen > 0 (num_solar); note that this count is just of the
number of landunits that this processor is responsible for; thus, this is where
the # PE-dependence comes in, I think.

(2) Later in that subroutine, a bunch of calculations are done if num_solar > 0
-- i.e., if this PE is responsible for at least one urban landunit with coszen

  1. Note that many of these calculations are done for all landunits, even ones
    for which coszen = 0. This introduces the possibility for different results
    depending on the decomposition.

(3) The particular difference that I am seeing is in albgrd & albgri. These are
initialized to 0 at the start of the subroutine, and so remain 0 on any PE for
which num_solar = 0. However, for PEs with num_solar > 0, landunits that have
coszen = 0 end up getting albgrd = albgri = 1. This is because the calculation
of albgrd & albgri depends on the values of the sref_* variables, which are
initialized to 1 (and stay at 1 for any landunit for which coszen = 0).

I have confirmed this by comparing the following from clm4_5_20: SMS_Lm1_P180x1.f19_g16.ICLM45BGC.yellowstone_intel vs SMS_Lm1_P360x1.f19_g16.ICLM45BGC.yellowstone_intel - both with ALBGRD, ALBGRI, ALBD and ALBI added to history output.

One thing I am confused about is whether this problem occurs from albgrd and albgri, but not for albd and albi - since the latter seem to just be copies of the former in UrbanMod.

@billsacks billsacks added this to the future milestone Dec 16, 2017
@billsacks
Copy link
Member Author

Bill Sacks < sacks@ucar.edu > - 2016-06-13 14:23:02 -0600

Keith: is this a priority to fix for CLM5? It affects these two diagnostic fields.

@billsacks
Copy link
Member Author

Keith Oleson < oleson@ucar.edu > - 2016-06-13 14:47:20 -0600

I don't think it's a priority. As I understand it, it just affects those history fields (which are off by default) and not the model operation (as in actual albedos that the urban model uses and/or bfb restarts).

@billsacks
Copy link
Member Author

Bill Sacks < sacks@ucar.edu > - 2016-06-13 14:58:17 -0600

Sounds good - thanks. Yes, your understanding is correct.

billsacks added a commit that referenced this issue Dec 28, 2017
(This has been copied to pull request #190 from
ESCOMP/vic_no_inf_excess_runoff)

Remove infiltration excess runoff for VIC

Martyn Clark reviewed the VIC implementation, and felt that the current
implementation of infiltration excess runoff is inconsistent with the
standard VIC implementation. It appears that what was being called VIC's
infiltration excess runoff was actually just an attempt to give a better
numerical approximation to the solution for saturated surface excess
runoff. So deleting this leaves only a first-order approximation to
VIC's saturated surface excess runoff.

Eventually we may want to put in place a more accurate solution for
VIC's saturated surface excess runoff. But Martyn's feeling is that this
can come in with other changes we want to make regarding numerical
solutions in CTSM.
@billsacks billsacks added type: bug bug something is working incorrectly and removed type: bug - science labels Feb 8, 2018
@billsacks billsacks removed this from the future milestone Nov 5, 2018
@billsacks
Copy link
Member Author

billsacks commented Jun 12, 2019

@olyson I propose to just remove these problematic fields. Do you object? If you want to keep them, would you have time to work on this issue at some point?

(If @olyson is okay with this plan, then I'll label this as "simple bfb".)

@billsacks billsacks added the priority: low Background task that doesn't need to be done right away. label Jun 12, 2019
@olyson
Copy link
Contributor

olyson commented Jun 12, 2019

I don't object to just removing these fields, thanks.

@billsacks billsacks changed the title albgrd and albgri history fields depend on decomposition, for urban points albgrd and albgri history fields depend on decomposition, for urban points; remove these fields Jun 13, 2019
@billsacks billsacks changed the title albgrd and albgri history fields depend on decomposition, for urban points; remove these fields albgrd and albgri history fields depend on decomposition, for urban points; remove these variables Jun 13, 2019
@billsacks billsacks changed the title albgrd and albgri history fields depend on decomposition, for urban points; remove these variables albgrd and albgri history fields depend on decomposition, for urban points Jun 13, 2019
@billsacks
Copy link
Member Author

Actually, I think I see a way to fix this problem that could also be good for performance. And, looking back at the code, I'd really like to remove this num_solar > 0 conditional. See #747

Blocked by #747

@billsacks billsacks added the blocked: dependency Wait to work on this until dependency is resolved label Jun 13, 2019
MiCurry pushed a commit to MiCurry/CTSM that referenced this issue Sep 16, 2021
update buildlib for upcoming cime/master
slevis-lmwg pushed a commit to slevis-lmwg/ctsm that referenced this issue Dec 22, 2022
slevis-lmwg pushed a commit to slevis-lmwg/ctsm that referenced this issue Dec 22, 2022
slevis-lmwg pushed a commit to slevis-lmwg/ctsm that referenced this issue Aug 1, 2023
Changes to fix #10, where an integer divide needs the floor operator
mvertens pushed a commit to mvertens/ctsm that referenced this issue Dec 8, 2023
Added description of clm-Nor-dev branch in README.md
samsrabin pushed a commit to samsrabin/CTSM that referenced this issue Apr 19, 2024
* remove FoX depdencies
* remove fox submodule
* update streamfilename

Co-authored-by: Jun Wang <junwang-noaa@users.noreply.github.com>
jedwards4b added a commit to jedwards4b/ctsm that referenced this issue Jun 2, 2024
AGonzalezNicolas pushed a commit to HPSCTerrSys/clm5_0 that referenced this issue Jun 27, 2024
(This has been copied to pull request ESCOMP#190 from
ESCOMP/vic_no_inf_excess_runoff)

Remove infiltration excess runoff for VIC

Martyn Clark reviewed the VIC implementation, and felt that the current
implementation of infiltration excess runoff is inconsistent with the
standard VIC implementation. It appears that what was being called VIC's
infiltration excess runoff was actually just an attempt to give a better
numerical approximation to the solution for saturated surface excess
runoff. So deleting this leaves only a first-order approximation to
VIC's saturated surface excess runoff.

Eventually we may want to put in place a more accurate solution for
VIC's saturated surface excess runoff. But Martyn's feeling is that this
can come in with other changes we want to make regarding numerical
solutions in CTSM.
AGonzalezNicolas pushed a commit to HPSCTerrSys/clm5_0 that referenced this issue Jun 27, 2024
Commit summary:

* 9291924 Integrated ParFlow psi into eCLM processes (preliminary changes)
* 9870b27 Removed optional argument `var_noshape` in oasis_def_var
* b208107 Removed optional argument var_noshape in oasis_def_var
* 1a6fbe1 Received soil liquid from ParFlow
* 07aa9de Added new soilwater_movement_method = coupled_parflow
* f6ae4d7 Temporarily bypass water and soil energy balance errors
* 506f160 Moved ParFlow subsurface flux calculations into a subroutine (ParFlowDrainage)
* b5094cc Converted ET fluxes [mm/s] to ParFlow flux units [1/hr]
* be01317 Ignore balance errors when use_parflow_soilwater=TRUE
* 602722c Merge branch 'master' into devel-parflow
AGonzalezNicolas pushed a commit to HPSCTerrSys/clm5_0 that referenced this issue Jun 27, 2024
Model developments by @s-poll :

* 9f00031 introduce precompiler COUP_OAS_ICON and COUP_OAS_PFL
* 59702f3 Oasis define for ICON ()
* a98d2cd Oasis snd/rcv for ICON (CLM3 vars)
* 1641475 consistent coupler precompiler naming
* f3c0cad include oas_receive_icon in eclm time stepping
* f7be360 renaming of coupled ICON-ECLM vars in oas_defineMod
* 27d984d Update snd/rcv fields from ICON
* ed1af99 Implementation of coupled variable t_sf.
* 47c2068 Couple rain_rate and snow_rate in seperate variables.
* 60850cf Restructure oasis_def_var.
* a42a3eb Inclusion of urban landunit.

Merges and bugfixes by @kvrigor :

* d79df54 Merge clm5nl-gen v0.6 (ESCOMP#16)
* 8ffe78f Merge eCLM-ParFlow coupling (ESCOMP#17)
* dfb6e69 Added compile definition COUP_OAS_PFL for ParFlow
* 0263ae8 Removed `use_parflow_soilwater()` in `BalanceCheckMod.F90` and `TotalWaterAndHeatMod.F90`
* 9ea5952 Fixed missing `psit` calculation when not coupled with ParFlow

Co-authored-by: Stefan Poll <spoll@users.noreply.github.com>
AGonzalezNicolas added a commit to HPSCTerrSys/clm5_0 that referenced this issue Jun 27, 2024
AGonzalezNicolas pushed a commit to HPSCTerrSys/clm5_0 that referenced this issue Jul 5, 2024
(This has been copied to pull request ESCOMP#190 from
ESCOMP/vic_no_inf_excess_runoff)

Remove infiltration excess runoff for VIC

Martyn Clark reviewed the VIC implementation, and felt that the current
implementation of infiltration excess runoff is inconsistent with the
standard VIC implementation. It appears that what was being called VIC's
infiltration excess runoff was actually just an attempt to give a better
numerical approximation to the solution for saturated surface excess
runoff. So deleting this leaves only a first-order approximation to
VIC's saturated surface excess runoff.

Eventually we may want to put in place a more accurate solution for
VIC's saturated surface excess runoff. But Martyn's feeling is that this
can come in with other changes we want to make regarding numerical
solutions in CTSM.
@samsrabin samsrabin added simple bfb bit-for-bit and removed simple bfb labels Aug 8, 2024
@samsrabin samsrabin added good first issue simple; good for first-time contributors and removed simple labels Oct 3, 2024
@ekluzek ekluzek linked a pull request Nov 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit blocked: dependency Wait to work on this until dependency is resolved bug something is working incorrectly good first issue simple; good for first-time contributors priority: low Background task that doesn't need to be done right away.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants