Skip to content

Conversation

@lwcugb
Copy link

@lwcugb lwcugb commented Mar 27, 2025

Adding GOCART DMS scheme based on PR #115

Note that this branch builds on SUV PR #117, and it should be merged first.


!NDMS = 1 is moved to scheme module
allocate(SU_emis(1,1,1)); SU_emis = ZERO

Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these dimensions representing? Can we add a comment for this?

!! \param delp Pressure Thickness for layer [Pa]
!! \param dmso_conc DMS source concentration [nmol/L]
!! \param SU_emis SU emissions, kg/m2/s
!! \param ndms index of DMS relative to other sulfate tracers
Copy link
Collaborator

@rschwant rschwant Apr 28, 2025

Choose a reason for hiding this comment

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

This is the only local variable defined here. Should this include local variable definitions too? And if so, for consistency, should we add fMassDMS too and others? Or is it best to not include local variables here?

Copy link
Author

Choose a reason for hiding this comment

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

We should only include the input and output variables to the subroutine here. ndms should not be here and has been deleted now.

! Local Variables
INTEGER, parameter :: NDMS = 1 ! index of DMS relative to other sulfate tracers
REAL, parameter :: fMassDMS=62. ! DMS molecular weight [g/mol]
character(len=256) :: errMsg
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we confirm that the MW of DMS in GOCART only uses 62. as opposed to more precision? We should eventually have the MW stored in one place of the code and update this to refer to the mechanism files, but this can be done later.

Copy link
Author

Choose a reason for hiding this comment

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

It is 62 indeed. The MW of DMS in GOCART is defined here. We do have MW in the species yaml file, but I am not sure if it should also be added to the emission yaml file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes it shouldn't be in the emission yaml but rather take it from the species yaml

Copy link
Author

Choose a reason for hiding this comment

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

Done!

!! \param TotalEmission Total emission of all species at each level [kg/m^2/s]
!! \param EmissionPerSpecies Emission per species at each level [kg/m^2/s]
!! \param FileDir Input file directory for reading in emissions
!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update this group of comments to include more spacing between the parameter name and description. Also Scheme looks to actually be SchemeOpt below.

real(fp), allocatable :: TotalEmission(:) !< Total emission of all species at each level [kg/m^2/s]
real(fp), allocatable :: EmissionPerSpecies(:,:) !< Emission per species at each level [kg/m^2/s]
character(len=1055) :: FileDir !< Input file directory for reading in emissions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again I would update this to have all capitals so that it looks more consistent. It looks messy to have both INTEGER and integer for example in the same place of the code.

!-----------------
TYPE(ConfigType) :: Config ! Module options
TYPE(EmisStateType) :: EmisState ! Chemical state

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the comment here should be Emis state and not Chemical state

TYPE(VolcanicStateType), INTENT(INOUT) :: VolcanicState !< VolcanicState Instance
!TYPE(ChemStateType), INTENT(INOUT) :: ChemState !< ChemState Instance
TYPE(EmisStateType), INTENT(INOUT) :: EmisState !< ChemState Instance

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we ever need the DiagState or ChemState here? If not, we should delete for clarity.

INTEGER :: i ! loop index
INTEGER :: hms ! Model time [secs] TODO: format is right?
INTEGER :: ymd ! Model date [YYYYMMDD] TODO: format is right?
CHARACTER(LEN=18) :: ymd_str ! Model date in string
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do these TODO notes mean? Do we need to confirm this still?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I am not sure if the format of date and time is correct for real model runs. It only works in this test.

REAL, dimension(:), allocatable :: vCloud ! top elevation of emissions [m]
REAL, dimension(:), allocatable :: vElev ! bottom elevation of emissions [m]
REAL, dimension(:), allocatable :: vLat ! latitude specified in file [degree]
REAL, dimension(:), allocatable :: VLon ! longitude specified in file [degree]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this should really be vLon to be consistent with that below and the other tracers.


REAL, dimension(:), allocatable :: vSO2 ! volcanic emissions [kg]
REAL, dimension(:,:,:),pointer :: SO2 ! SO2 [kg kg-1]
!REAL, dimension(:,:,:),pointer :: SU_emis ! SU emissions, kg/m2/s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you ever need this line? If not, let's delete it.

!we could pre-select sources for the current grid cell so giving all ones can work fine.
allocate(iPoint(nVolc), jPoint(nVolc))
iPoint = 1; jPoint = 1
!TODO: Not sure how to get Vstart and VEnd format and values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this comment and mention where the 235959 number comes from.

endif ! if (VolcanicState%SchemeOpt == 1)

write(*,*) 'TODO: Need to figure out how to add back to the chemical species state '

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we write down stuff like this that still needs to be done in all these routines? Maybe make a google doc for this.

Copy link
Author

Choose a reason for hiding this comment

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

That is a good idea. Right now I am using TODO to record these things.

!! \param nPts Number of volcanic sources in the current file
!! \param emissfile Emissions file name
!! \param label Label for emissions
!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we indent the description on these comments like that below so is cleaner and more consistent.

!! \param vSO2 Volcanic emissions [kg S/s]
!! \param nSO2 Index of SO2 relative to other sulfate tracers
!! \param SO2 SO2 emissions [kg kg-1]
!! \param SU_emis SU emissions, kg/m2/s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is SU_emis the only local variable defined here? Should we define all variables or leave out all local variables?

Copy link
Author

Choose a reason for hiding this comment

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

SU_emis should not be here. It is deleted now.

REAL, dimension(:,:,:),pointer :: SU_emis ! SU emissions [kg/m2/s]
REAL, dimension(:,:),pointer :: SO2EMVol ! volcanic emissions [kg m-2 s-1]
REAL, parameter :: fMassSulfur = 32. ! gram molecular weights of species
REAL, parameter :: fMassSO2 = 64. ! gram molecular weights of species
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again eventually need a place to define these all within the mechanism rather than here. Does GOCART only use low precision MWs fro these or should we add higher precision here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. GOCART uses lower precision. We should be able to use higher precision if we like.

map: ["dust1"]
DU10:
long_name: "Dust Species Coarse Mode"
map: ["dust1", "dust2"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about adding the MW in these yaml files instead of hardcoded in the code?

Copy link
Author

Choose a reason for hiding this comment

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

It should work if MW is needed for most of the emission species. Right now, we only have DMS and sulfur. The species yaml file already has MW included, although it can be different from the emission species.

@lwcugb lwcugb requested a review from bbakernoaa April 30, 2025 20:34
@bbakernoaa
Copy link
Collaborator

@lwcugb are all the comments resolved?

@lwcugb
Copy link
Author

lwcugb commented Jun 9, 2025

@lwcugb are all the comments resolved?

Yes. But as suggested, we can consider adding emission species MW in the yaml too as suggested.

@rschwant
Copy link
Collaborator

rschwant commented Jun 9, 2025

Let's update the hardcoded MWs to be based on the chemstate instead.

@lwcugb
Copy link
Author

lwcugb commented Jun 10, 2025

Let's update the hardcoded MWs to be based on the chemstate instead.

Done!

real, pointer :: GOCART_DELP(:,:,:)
character(len=256) :: errMsg
character(len=256) :: thisLoc

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to update these too? I guess this would be an example that if the emissions are in Sulfur with a MW of 32. and the model parameters is SO2. We could have the code do this conversion more generally. Rather than hard coding it for all options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request New Process Used for new processes

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

5 participants