Introduce SUBROUTINE GWspatialParameters #224
Conversation
|
@rkutteh I know you were hoping for a quick pull request. But I don't think I can give you that. From what I've seen during the review the GWspatialParameters add to the spaghetti mess of cable_parameters.F90 instead of helping untangling things. So I'm not particularly inclined to accept as is. I'll have a deeper look into it and see if we can adapt things quickly to get the code to a better standard. |
|
@ccarouge As you might guess from my earlier comments, I prefer that all clean-up of code occurs after all the GW work is in and validated against the copy I have outside the repo. If you change things around while I am moving code in and the final code does not agree with my results, it will be nearly impossible for me to track down the source(s) of the errors. As it stands, we know that the code that I am introducing works with GW on, so my own strategy would be to mirror it in the repo, check that all is good as expected, and only THEN clean-up and check, clean-up and check,...Keep in mind that GWspatialParameters is presently inactive. |
There was a problem hiding this comment.
I have written down what I would change (at least what I could see here).
But I have seen that cable_parameters.F90 already has code to read in the GW parameters and assign the values to the CABLE variables (in spatialSoil() and write_default_params()), so we need to ensure there is only one way to do that in the final code.
This clean up will be done separately (#229) once the GW code is committed in to ensure testing is possible.
| ! mpatch setting below introduced by rk4417 - phase2 | ||
| !set mpatch, the publically available number of patches | ||
| mpatch = npatch ! MMY@13April it makes sense. mpatch is set in cable_define_types_mod | ||
| ! as a public variable and is used in subroutine GWspatialParameters | ||
| ! and in get_gw_data | ||
|
|
There was a problem hiding this comment.
I don't see why this is needed since we reset to max_vegpatches below. Or did I miss a if statement?
| IF (nlon .lt. 0 .or. nlon .ne. xdimsize) & | ||
| WRITE(logn,*) 'nlon: found ',nlon,' need to have ',xdimsize,' to match forcing.' | ||
| IF (nlat .lt. 0 .or. nlat .ne. ydimsize) & | ||
| WRITE(logn,*) 'nlat: found ',nlat,' need to have ',ydimsize,' to match forcing.' | ||
| IF (nhorz .lt. 0 .or. nhorz .ne. ms) & | ||
| WRITE(logn,*) 'nhorz: found ',nhorz,' need to have ',ms,' to match forcing.' |
There was a problem hiding this comment.
The negative tests (.lt. 0) are unnecessary as xdimsize, ydimsize and ms will never be negative.
| IF( NF90_INQ_DIMID(ncid,'nhorz',layer_id) .eq. nf90_noerr) THEN | ||
| IF ( NF90_INQUIRE_DIMENSION(ncid,layer_id,LEN=nhorz) .ne. nf90_noerr) nhorz=1 | ||
| ELSE | ||
| nhorz=ms | ||
| ENDIF |
There was a problem hiding this comment.
It's weird that if that dimension is not found in the file (ELSE statement), we set it to the correct value. Shouldn't it be -1 in the ELSE?
| WRITE(logn,*) 'Setting dims to forcing file values, CHECK THE OUTPUT!' | ||
| !amu561: code should probably stop here instead of re-writing | ||
| !dimensions? | ||
| nlon=xdimsize; nlat=ydimsize; npatch=mpatch; nhorz=ms |
There was a problem hiding this comment.
Wrong indent and I agree with Anna, this should not be done. We should have an exit here to stop the code.
| ! MMY @Oct2022 give read-in value to non-vec parameters | ||
| do e=1,mland | ||
| soil%silt(landpt(e)%cstart:landpt(e)%cend) = & | ||
| insilt(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%sand(landpt(e)%cstart:landpt(e)%cend) = & | ||
| insand(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%clay(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inclay(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%swilt(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inswilt(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%sfc(landpt(e)%cstart:landpt(e)%cend) = & | ||
| insfc(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%ssat(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inssat(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%bch(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inbch(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%hyds(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inhyds(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%sucs(landpt(e)%cstart:landpt(e)%cend) = & | ||
| -1.* ABS(insucs(landpt(e)%ilon, landpt(e)%ilat)) !ensure negative | ||
| soil%rhosoil(landpt(e)%cstart:landpt(e)%cend) = & | ||
| inrhosoil(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%css(landpt(e)%cstart:landpt(e)%cend) = & | ||
| incss(landpt(e)%ilon, landpt(e)%ilat) | ||
| soil%cnsd(landpt(e)%cstart:landpt(e)%cend) = & | ||
| incnsd(landpt(e)%ilon, landpt(e)%ilat) | ||
| ! 26 | ||
| ! soil%GWrhosoil_vec(landpt(e)%cstart:landpt(e)%cend) = & ! MMY not used | ||
| ! REAL(inGWrhosoil(landpt(e)%ilon, landpt(e)%ilat),r_2) | ||
| end do | ||
|
|
There was a problem hiding this comment.
This has nothing to do with reading the GW parameters file. It should not be done in this subroutine. It should be done when dealing with derived parameters.
| if (.not.cable_user%gw_model) then | ||
|
|
||
| GW2d_data(:,:) = default_data(:,:) | ||
|
|
||
| else |
There was a problem hiding this comment.
I would like to remove this case. This should be done outside these routines. Keep these generic.
| if (.not. cable_user%gw_model) then | ||
| do k=1,ms | ||
| GW3d_data(:,:,k) = default_data(:,:) | ||
| end do | ||
|
|
||
| else |
There was a problem hiding this comment.
I would like to remove this case. This should be done outside these routines. Keep these generic.
| if (.not. cable_user%gw_model) then | ||
| do k=1,ms | ||
| GW3d_data(:,:,k) = default_data(:,:) | ||
| end do | ||
|
|
||
| else |
There was a problem hiding this comment.
I would like to remove this case. This should be done outside these routines. Keep these generic.
| if (.not.cable_user%gw_model) then | ||
| do k=1,ms | ||
| do n=1,npatch | ||
| GW4d_data(:,:,k,n) = default_data(:,:) | ||
| end do | ||
| end do | ||
| else |
There was a problem hiding this comment.
I would like to remove this case. This should be done outside these routines. Keep these generic.
| if (.not.cable_user%gw_model) then | ||
| do k=1,ms | ||
| do n=1,npatch | ||
| GW4d_data(:,:,k,n) = default_data(:,:) | ||
| end do | ||
| end do | ||
| else |
There was a problem hiding this comment.
I would like to remove this case. This should be done outside these routines. Keep these generic.
|
@ccarouge thanks for all the comments. Some I suspect will be resolved "automatically", once I am finished pushing all the remaining changes to src/offline/cable_parameters.F90 . The remaining I agree we can address after testing. |
CABLE ground water hydrology work of Mengyuan Mu
This pull request is part of integrating the ground water hydrology work of Mengyuan Mu into CABLE, which will take place in a sequence of pull requests. The actual integration project itself was completed back in September 2023. The current work involves just a gradual merge into the main branch.
Description
📚 Documentation preview 📚: https://cable--224.org.readthedocs.build/en/224/