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

problem with the bounds of some history fields #16

Open
billsacks opened this issue Dec 16, 2017 · 4 comments
Open

problem with the bounds of some history fields #16

billsacks opened this issue Dec 16, 2017 · 4 comments
Labels
code health improving internal code structure to make easier to maintain (sustainability) priority: low Background task that doesn't need to be done right away.

Comments

@billsacks
Copy link
Member

Bill Sacks < [email protected] > - 2013-08-17 07:46:54 -0600
Bugzilla Id: 1786
Bugzilla CC: [email protected], [email protected], [email protected], [email protected],

There is a potential problem with the bounds of some history fields. My guess is that this doesn't cause any problems now, but could cause problems in the future, if either (1) hist_update_hbuf was called within a threaded region (right now it's not), or (2) assumptions were made about the lower bound of arrays in hist_update_hbuf.

The problem arises from associating a pointer with an array slice, as in:

ptr => target(:, 1:n)

When you do this, the lower bound of ptr is reset to 1. Contrast this to:

ptr => target

in which case ptr retains the lower bounds of target.

Specifically, this occurs in:

(1) hist_update_hbuf_field_2d

field          => clmptr_ra(hpindex)%ptr(:,1:num2d)

(2) histFldsMod; e.g.:

      data1dptr => ccs%decomp_cpools(:,l)

(and maybe elsewhere - I haven't done an extensive search)

I believe this can be solved with the following syntax:

! In the following, we need to explicitly set the lower bound of 'field', otherwise
! it gets set to 1 when it's associated with the slice of 'ptr'
arr_lbound = lbound(clmptr_ra(hpindex)%ptr, 1)
field(arr_lbound: , 1:) => clmptr_ra(hpindex)%ptr(:,1:num2d)

but I haven't tested this.

For now, in the interest of time, I am working around this problem simply by NOT explicitly specifying the bounds of the history fields in calls to p2g/c2g/l2g in hist_update_hbuf; e.g., I am calling these routines like:

      call p2g(bounds, &
           field, &
           field_gcell(bounds%begg:bounds%endg), &
           p2c_scale_type, c2l_scale_type, l2g_scale_type)

rather than like:

      call p2g(bounds, &
           field(bounds%begp:bounds%endp), &
           field_gcell(bounds%begg:bounds%endg), &
           p2c_scale_type, c2l_scale_type, l2g_scale_type)

I think this should be okay for now, but wouldn't work if this was called from within a threaded region.

@billsacks billsacks added this to the future milestone Dec 16, 2017
@billsacks billsacks added the priority: low Background task that doesn't need to be done right away. label Dec 16, 2017
@billsacks
Copy link
Member Author

Bill Sacks < [email protected] > - 2013-08-26 06:06:31 -0600

I think that the trickier part of this problem - the calls from histFldsMod (#2 in my original report) - can be solved by changing the dummy argument declarations in hist_addfld1d and hist_addfld2d, by adding lower bounds to these arguments:

real(r8)        , optional, pointer    :: ptr_gcell(:)   ! pointer to gridcell array
real(r8)        , optional, pointer    :: ptr_lunit(:)   ! pointer to landunit array
real(r8)        , optional, pointer    :: ptr_col(:)     ! pointer to column array
real(r8)        , optional, pointer    :: ptr_pft(:)     ! pointer to pft array

(I missed these in my initial rework of subroutine arguments.)

To do this, we would need to pass bounds to hist_addfld1d and hist_addfld2d - ensuring that these are the proc bounds rather than the clump bounds.

At that point, I believe that the only remaining problem would be #1 in my initial report, which could be solved via my original suggestion.

I'm not sure whether this refactoring is worth the time right now, though.

@billsacks
Copy link
Member Author

Bill Sacks < [email protected] > - 2013-09-19 14:31:41 -0600

I am adding more code in histFileMod where this issue arises - specifically, code to deal with multi-layer snow history fields.

Specifically, I have this code on my branch, in hist_update_hbuf_field_2d:

if (is_snow_layer_field) then
   ! For multi-layer snow fields, build a special output variable that handles
   ! missing snow layers appropriately

   ! Note that the following allocation is not what we would want if this routine
   ! were operating in a threaded region (or, more generally, within a loop over
   ! nclumps) - in that case we would want to use the bounds information for this
   ! clump. But currently that's not possible because the bounds of some fields have
   ! been reset to 1 - see also bug 1786.
   allocate(field(lbound(clmptr_ra(hpindex)%ptr, 1) : ubound(clmptr_ra(hpindex)%ptr, 1), 1:num2d))

Ideally, we would want field to be allocated to be just big enough for the given clump bounds (if we were within a loop over clumps). But that's not possible if the field's lower bound has been reset to 1.

So, as with my above notes, this code will only work as intended if the bounds passed to hist_update_hbuf_field_2d are proc bounds.

Once this bug is addressed, we should search for '1786' in histFileMod to find all notes about this problem.

@billsacks
Copy link
Member Author

Bill Sacks < [email protected] > - 2013-09-19 14:32:41 -0600

It looks like the syntax I suggested:

field(arr_lbound: , 1:) => clmptr_ra(hpindex)%ptr(:,1:num2d)

is a fortran2003 feature, which should now be supported by the major compilers.

@billsacks
Copy link
Member Author

Bill Sacks < [email protected] > - 2016-05-16 14:26:27 -0600

Probably deferring this until threading is reworked in CLM to be done at a higher level - at which point this bug will either go away or be changed considerably

@billsacks billsacks added the code health improving internal code structure to make easier to maintain (sustainability) label Feb 8, 2018
@billsacks billsacks removed this from the future milestone Nov 5, 2018
MiCurry pushed a commit to MiCurry/CTSM that referenced this issue Sep 16, 2021
update horiz_grid and topo files for MOM6 grid.
slevis-lmwg referenced this issue in slevis-lmwg/ctsm Aug 1, 2023
Fix cold start and replace mosart name
samsrabin pushed a commit to samsrabin/CTSM that referenced this issue Apr 19, 2024
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
Commit summary:

* 437789c48 Bumped clm5nl-gen version to 0.6
* b35675172 Wrapped lines based on max column width
* 0b3e1b53c Automatically set user-specified namelist parameters
* bb5caa29f Made reading/writing/checking valid namelist parameters more convenient 
* 13286e75f Fixed __contains__ and __str__ in NamelistGroupMixin
* 7ef720dee Removed <nl>.user_nl and introduced <nl>.general_options
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 <[email protected]>
AGonzalezNicolas added a commit to HPSCTerrSys/clm5_0 that referenced this issue Jun 27, 2024
slevis-lmwg added a commit to olyson/ctsm that referenced this issue Nov 15, 2024
…e_bounds-ssr

Handle "instantaneous files" in RXCROPMATURITY analysis script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability) priority: low Background task that doesn't need to be done right away.
Projects
None yet
Development

No branches or pull requests

1 participant