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.

Sign up for GitHub

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

Jump to bottom

Merging Active Sensor and DDA Cloud Coefficients into V3 #39

Merged
merged 27 commits into from
Oct 18, 2023
Merged

Merging Active Sensor and DDA Cloud Coefficients into V3 #39

merged 27 commits into from
Oct 18, 2023

Conversation

imoradi
Copy link
Collaborator

@imoradi imoradi commented Jun 20, 2023 •
edited by StegmannJCSDA
Loading

Description

This will merge active sensor module and also the code changes to be able to run CRTM using the DDA hydrometeor lookup tables.

Issue(s) addressed

Dependencies

List the other PRs that this PR is dependent on:

I have already merged the changes from other PR #37 into this PR.

Impact

This should make CRTM V3.0 able to compute reflectivities and work with the DDA lookup tables.

Checklist

  • I have run this version for both active sensor to compute reflectivities and also run new cloud lookup tables. I have also compared the computed reflectivities from V2.4.1 and they are identical.

imoradi and others added 12 commits June 14, 2023 19:42
@imoradi
merging active sensor module
a5a2c39
@imoradi
Update CloudCoeff_Define.f90
6ae7b3c 
Some lines are missing after we merged the changes
@imoradi
Update CloudCoeff_Define.f90
f5fb1d9 
additional changes from V2.4 to V3.0
@imoradi
Update CRTM_Atmosphere_Define.f90
dc9847e
@imoradi
Update make.dependencies
5ad0182
@imoradi
Update CRTM_Forward_Module.f90
e7d3940
@imoradi
Merge branch 'JCSDA:develop' into develop
7e5c576
@imoradi
Merge branch 'develop' into feature/cd_v241_cloud_aerosol_io
0376539
@imoradi
Merge pull request #1 from imoradi/feature/cd_v241_cloud_aerosol_io
55e8a56 
Feature/cd v241 cloud aerosol io
@imoradi
Update CloudCoeff_Define.f90
c6893d6 
n_IR_Densities is separated so we don't need to define it from zero anymore when used as a dimension
@imoradi
Update CloudCoeff_Define.f90
079a69b 
More changes due to separating n_IR_Densities from that of MW so that we can define IR elements from 1 instead of 0
@imoradi
added lines for AerosolCoeff_File and CloudCoeff_File
4e041a9
@imoradi imoradi self-assigned this Jun 20, 2023
@StegmannJCSDA StegmannJCSDA added enhancement New feature or request CRTM CRTM duplicate This issue or pull request already exists labels Jun 20, 2023
@StegmannJCSDA
Copy link
Contributor

@imoradi testing now.

@StegmannJCSDA
Copy link
Contributor

test_AD_Active_Sensor.f90 fails to build with the following message from gfortran:

CRTMv3/test/mains/unit/Unit_Test/test_AD_Active_Sensor.f90:405:4:

  405 |     RTSolution_AD%Reflectivity_Attenuated(jj) = ZERO
      |    1
Error: Component to the right of a part reference with nonzero rank must not have the ALLOCATABLE attribute at (1)
CRTMv3/test/mains/unit/Unit_Test/test_AD_Active_Sensor.f90:406:4:

  406 |     RTSolution_AD%Reflectivity(jj) = ZERO
      |    1
Error: Component to the right of a part reference with nonzero rank must not have the ALLOCATABLE attribute at (1)
CRTMv3/test/mains/unit/Unit_Test/test_AD_Active_Sensor.f90:408:7:

  408 |        RTSolution_AD(ichan,:)%Reflectivity_Attenuated(jj) = ONE
      |       1
Error: Component to the right of a part reference with nonzero rank must not have the ALLOCATABLE attribute at (1)
CRTMv3/test/mains/unit/Unit_Test/test_AD_Active_Sensor.f90:410:7:

  410 |        RTSolution_AD(ichan,:)%Reflectivity(jj) = ONE
      |       1
Error: Component to the right of a part reference with nonzero rank must not have the ALLOCATABLE attribute at (1)

@imoradi
Copy link
Collaborator Author

imoradi commented Jun 20, 2023 via email

@StegmannJCSDA
Copy link
Contributor

Are you testing it on S4? If so then can you share with me your gfort.setup

No, I am testing locally and I am using the ecbuild build.

@imoradi
Copy link
Collaborator Author

imoradi commented Jun 21, 2023

Can someone point me to the docs showing how to load ecbuild modules for gfort on S4?

imoradi added 2 commits June 21, 2023 17:39
@imoradi
Update test_AD_Active_Sensor.f90
3561858 
Fixed the issues with gfort that does not allow initializing an array using a scalar.
@imoradi
Update test_AD_Active_Sensor.f90
e3f7a73 
Added Active_Sensor flag
@imoradi
Copy link
Collaborator Author

imoradi commented Jun 21, 2023

This is probably a gfort issues not allowing to initialize an array using an scalar (ifort is fine). Can you try test_AD_Active_Sensor.f90 to see if it compiles now?

StegmannJCSDA
StegmannJCSDA suggested changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@StegmannJCSDA StegmannJCSDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is very extensive again and the following tests fail:

	 41 - test_k_matrix_Simple_atms_n21 (SEGFAULT)
	 42 - test_k_matrix_Simple_cris-fsr_n21 (SEGFAULT)
	 43 - test_k_matrix_Simple_v.abi_g18 (SEGFAULT)
	 44 - test_k_matrix_Simple_modis_aqua (SEGFAULT)
	 72 - test_adjoint_Simple_atms_n21 (Failed)
	 73 - test_adjoint_Simple_cris-fsr_n21 (Failed)
	 74 - test_adjoint_Simple_v.abi_g18 (Failed)
	 75 - test_adjoint_Simple_modis_aqua (Failed)
	 88 - test_Active_Sensor (Failed)
	 90 - test_TL_convergence_active_sensor (Failed)

The only ctest that hasn't been fixed yet is test_Active_Sensor, which fails because the following file still needs to be added to the testing data before the PR can be merged:
./testinput/CloudCoeff_DDA_ARTS.nc4.

.gitignore Outdated
@@ -1,2 +1,11 @@
.DS_Store
*.o
*.rej
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't really have to deal with *.rej files.

src/Build/CMakeFiles/CMakeOutput.log Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file needs to be removed.

src/Coefficients/CloudCoeff/CloudCoeff_Define.f90 Outdated
DO i = 1, CloudCoeff%n_IR_Radii
WRITE(*,'(7x,"Effective radius: ",es13.6)') CloudCoeff%Reff_IR(i)
WRITE(*,'(5(1x,es13.6,:))') CloudCoeff%g_IR(:,i,j)
END DO
END DO

IF ( wait ) THEN
WRITE(*,'(/5x,"Press <ENTER> to view the infrared solid phase backscatteringparameter")')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no space in backscatteringparameter.

BenjaminTJohnson
BenjaminTJohnson requested changes Jul 11, 2023
View reviewed changes
src/AtmScatter/CRTM_CloudScatter.f90
@@ -317,7 +363,11 @@ FUNCTION CRTM_Compute_CloudScatter( &
! compute the single scatter albedo in the Layer_loop below.
CScat%Optical_Depth(kc) = CScat%Optical_Depth(kc) + &
(CSV%ke(kc,n)*Atm%Cloud(n)%Water_Content(kc))

IF ( SC(SensorIndex)%Is_Active_Sensor .and. CScat%Include_Scattering ) THEN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use of a flag is inconsistent with:

USE CRTM_SpcCoeff,            ONLY: SC, &
SpcCoeff_IsMicrowaveSensor , & 
SpcCoeff_IsInfraredSensor  , &
SpcCoeff_IsVisibleSensor   , &
SpcCoeff_IsUltravioletSensor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we discussed this when merging into V2.4.1 - basically the active sensors can be MW, IR or anything so we need to see whether a sensor is MW/IR/etc first then active or passive.

src/AtmScatter/CRTM_CloudScatter.f90
CSV%w(kc,n) , & ! Input
SC(SensorIndex)%Is_Active_Sensor , & ! Input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be treated exactly the same as other sensor types, such as SpcCoeff_IsMicrowaveSensor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my previous comment. Active sensors still need to be one of the other kinds.

src/AtmScatter/CRTM_CloudScatter.f90
CSV%w(kc,n) , & ! Input
SC(SensorIndex)%Is_Active_Sensor , & ! Input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't comment further, but throughout this needs to be consistent.

src/AtmScatter/CRTM_CloudScatter.f90 Outdated
END IF
END IF ! ICE_Cloud etc interpol
CASE DEFAULT
print*, 'Cloud State not defined'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the built-in message system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Atmosphere/CRTM_Atmosphere.f90
@@ -460,6 +472,11 @@ FUNCTION CRTM_Atmosphere_AddLayers_TL( &
! Set up
err_stat = SUCCESS

IF (.NOT. Atm_In%Add_Extra_Layers) THEN
! This will just copy Atm_In_TL to Atm_Out_TL
atm_out_TL = CRTM_Atmosphere_AddLayerCopy( Atm_In_TL, 0 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a bug fix -- was this something specific that you found?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the active module we don't want CRTM to mess with the input layers so I have added this flag to ensure that the output reflectivities are at the same level as the input profiles.

src/RTSolution/CRTM_Active_Sensor.f90
tc = 133.1383

e0 = 87.9144
e1 = 0.404399_fp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do some constants get _fp and others don't?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added _fp to other decimals

src/RTSolution/CRTM_Active_Sensor.f90
ENDIF
dZ_m = (Atm%Height(0:Atm%n_Layers-1) - Atm%Height(1:Atm%n_Layers)) * ONE_THOUSAND
dZ_m = dZ_m / GeometryInfo%Cosine_Sensor_Zenith

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again check for dZ_m > 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as the pressure is different then dz_m will not be zero and the profiles pressure levels are checked before hands. Why do we want to double check when it should have been checked from the first place? It is easy to put a statement here but just want to make sure it is really necessary.

src/RTSolution/CRTM_Active_Sensor.f90
perm = Water_Permittivity_Turner_2016(Frequency * 1.0d9, & ! Input
Temp_K) ! Input

perm_re = REAL(REAL(perm)) ! Double REAL is required to avoud issues with GNU Fortran
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the issue? I've never seen this before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this link https://gcc.gnu.org/onlinedocs/gcc-3.4.6/g77/REAL_0028_0029-and-AIMAG_0028_0029-of-Complex.html

src/RTSolution/CRTM_Active_Sensor.f90
SensorIndex , & ! Input
ChannelIndex, & ! Input
RTSolution ) ! Input/Output
! Arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all subroutines should start with implicit none

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have included IMPLICIT NONE in the top of the module and that should cover all the subroutines. Isn't it? I believe that is the case but if not then will it to each subroutines. I also checked other modules of CRTM and it has only being added once - see also this link https://stackoverflow.com/questions/24337413/where-to-put-implicit-none-in-fortran

src/RTSolution/CRTM_Active_Sensor.f90
kw = (perm - ONE )/(perm + TWO)
Kw_2 = ABS(kw)**TWO

P1 = (M6_MM6 * Wavelength_m**4.0_fp) / (PI**5.0_fp * Kw_2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is M6_MM6 defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same file as private variables only accessible to the module subroutines.

@BenjaminTJohnson
Copy link
Contributor

@imoradi finished the first round of review, I haven't tested the code yet.

imoradi added 2 commits July 11, 2023 00:06
@imoradi
Update CRTM_CloudScatter.f90
6d73d96 
Changed the message handler from PRINT to CRTM Internal Message handler
@imoradi
Update CRTM_Active_Sensor.f90
9f4c027 
Some minor changes by adding _fp to decimals
@imoradi
Copy link
Collaborator Author

imoradi commented Jul 11, 2023

@imoradi finished the first round of review, I haven't tested the code yet.

Thanks @BenjaminTJohnson - I have responded to your comments. Some of the points here were also discussed when merging this into V2.4.1.

@BenjaminTJohnson
Copy link
Contributor

BenjaminTJohnson commented Jul 17, 2023 •
edited
Loading

In subroutine (Get_Cloud_Opt_MW):

crtm/src/AtmScatter/CRTM_CloudScatter.f90(1406): error #6404: This name does not have a type, and must have an explicit type.   [ERROR_STATUS]
          Error_Status = FAILURE
----------^

It's missing:

INTEGER :: Error_Status 
<...>
 Error_Status = SUCCESS
<...>

You'll also need a variable for the Message:

    ! Local variables
    CHARACTER(ML) :: Message

and ROUTINE_NAME:

    ! Local parameters
    CHARACTER(*), PARAMETER :: ROUTINE_NAME = 'Get_Cloud_Opt_MW'

@BenjaminTJohnson
Copy link
Contributor

@imoradi This would be much easier to merge if you created a feature branch in the CRTMv3 repository instead of trying to merge from your forked repository, that way I could hotfix your feature branch for minor issues (such as above) to accelerate the process.

@chengdang chengdang mentioned this pull request Jul 18, 2023
Merged
@BenjaminTJohnson
Copy link
Contributor

@imoradi please merge the latest CRTMv3 develop, and resolve the noted conflicts.
Thanks!

@imoradi
Copy link
Collaborator Author

imoradi commented Jul 22, 2023

@imoradi please merge the latest CRTMv3 develop, and resolve the noted conflicts.

Unfortunately the entire "src/CRTM_Forward_Module.f90" needs once again to be manually checked and merged as changes from the features that were opened after I made this PR are scattered all over this file. I already merged one of the PRs that fixes the aerosol coefficient issue. This was the issue preventing me from using my CRTMv3.0 fork for the JEDI developments.

@BenjaminTJohnson BenjaminTJohnson mentioned this pull request Jul 25, 2023
Closed
8 tasks
fabiolrdiniz referenced this pull request Sep 22, 2023
@fabiolrdiniz
threads settings for zero profile case
8df1a2b
@imoradi imoradi mentioned this pull request Sep 22, 2023
Merged
@BenjaminTJohnson BenjaminTJohnson merged commit 0ee8a41 into JCSDA:develop Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StegmannJCSDA StegmannJCSDA StegmannJCSDA requested changes

@BenjaminTJohnson BenjaminTJohnson BenjaminTJohnson requested changes

Assignees

@imoradi imoradi

Labels
CRTM CRTM duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add DDA hydrometeor LUT to CRTMv3 Add active sensor module to CRTMv3
3 participants
@imoradi @StegmannJCSDA @BenjaminTJohnson