Abstract aerosol interfaces extended for bulk representation #1404
Conversation
modified: src/chemistry/aerosol/aerosol_properties_mod.F90 modified: src/chemistry/aerosol/carma_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/modal_aerosol_properties_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
new file: src/physics/cam/bam_optics_diags_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/hygrocoreshell_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/hygroscopic_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/insoluble_aerosol_optics_mod.F90 modified: src/physics/cam/aerosol_optics_cam.F90
new file: src/chemistry/aerosol/hygro_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/aerosol_properties_mod.F90 modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/carma_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/hygroscopic_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/modal_aerosol_properties_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/hygro_aerosol_optics_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
new file: src/chemistry/aerosol/volcrad_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/aerosol_properties_mod.F90 modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/carma_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/modal_aerosol_properties_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/bam_optics_diags_mod.F90
modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aerosol_optics_cam.F90
renamed: src/physics/cam/bam_optics_diags_mod.F90 -> src/physics/cam/aer_vis_diag_mod.F90 modified: src/physics/cam/aer_rad_props.F90 modified: src/physics/cam/aer_vis_diag_mod.F90 modified: src/physics/cam/aerosol_optics_cam.F90
modified: src/physics/cam/aer_rad_props.F90
modified: src/physics/cam/aerosol_optics_cam.F90
modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/hygro_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/hygrocoreshell_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/hygroscopic_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/insoluble_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/volcrad_aerosol_optics_mod.F90 modified: src/physics/cam/aer_vis_diag_mod.F90
|
|
||
| nspecies(:) = 1 | ||
|
|
||
| alogsig(:) = log(2._r8) |
There was a problem hiding this comment.
Should this be made as a parameter and then assigned to alogsig?
There was a problem hiding this comment.
What will we gain be inserting a parameter here?
There was a problem hiding this comment.
What will we gain be inserting a parameter here?
I'm just not sure where this magic number is coming from, so a comment for at least the value would help. It seems to be the same value as CARMA, and MAM has species-specific deviations.
There was a problem hiding this comment.
This may never be used for bulk aerosols. The settings are adopted from CARMA properties. Comments are added.
| ! call endrun('ERROR: bulk_aerosol_properties_mod%amcube not yet implemented') | ||
| amcube = -huge(1._r8) |
There was a problem hiding this comment.
Maybe add a comment here that amcube cannot be defined for bulk aerosols since there are no number concentrations in the bulk representation?
I understand endrun cannot be called here because this implements a pure function so maybe a comment of why the intentionally invalid value is returned here will be useful, to distinguish between 'does not exist for bulk representation' and 'not yet implemented'
There was a problem hiding this comment.
It seems to me that the need to look for workarounds like this is a sign that something is wrong with the abstract aerosol class structure. In this case, the abstract properties class is requiring information that is not present in all aerosol models.
In a perfect world, all code that used %amcube (and the other problematic methods below) would reside in the appropriate subclass and all uses would be restricted to that subclass (which could still be a superclass of final models if a set of aerosol models shared a property such as amcube).
I don't see CAM (or CAM-SIMA) getting ready for such a clean class hierarchy soon so in the interim, I think it would be best if the abstract base class (aerosol_properties) provided a default, overridable method to produce an invalid value. I am happy to present this change as a PR to this PR branch so you can evaluate it.
Speaking of an invalid value, what happens if <bulk_aero_prop_obj>%amcube gets called? The code where it is called has no value checks.
There was a problem hiding this comment.
@jimmielin and @gold2718
Many of these undefined methods (such as amcube) could probably be implemented for bulk aerosols. They are not needed for optical properties of prescribed bulk aerosols, which is the purpose of this PR. In the interest of time, I did not make the effort. They can be implemented later if needed.
There was a problem hiding this comment.
@fvitt, thanks for the explanation. Would it be possible to set the value to NaN instead of -huge? That way we would know if any not-implemented method was used in active code.
There was a problem hiding this comment.
One aspect I struggled when reviewing the PR is that perhaps there could be better distinction of what is not implemented yet (as @fvitt notes as the scope of this PR) versus what cannot be physically defined for bulk aerosols (as @gold2718 notes as "information that is not present in all aerosol models"). Either -huge or NaN serve as good magic numbers for me but I believe the comments should make the intent of the code clear.
|
|
||
| nullify(num) | ||
|
|
||
| call endrun('ERROR: bulk_aerosol_state_mod%get_ambient_num not yet implemented') |
There was a problem hiding this comment.
I would suggest returning the error as this cannot be defined for bulk
| call endrun('ERROR: bulk_aerosol_state_mod%get_ambient_num not yet implemented') | |
| call endrun('ERROR: bulk_aerosol_state_mod%get_ambient_num cannot be defined for bulk representation') |
|
|
||
| nullify(num) | ||
|
|
||
| call endrun('ERROR: bulk_aerosol_state_mod%get_cldbrne_num not yet implemented') |
There was a problem hiding this comment.
Similarly I would suggest returning the error as this cannot be defined for bulk instead of not yet implemented (as it never will)
| call endrun('ERROR: bulk_aerosol_state_mod%get_cldbrne_num not yet implemented') | |
| call endrun('ERROR: bulk_aerosol_state_mod%get_cldbrne_num cannot be defined for bulk representation') |
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
I would like to see a bit of a shift in how the non-bulk methods are implemented (see below).
| ! call endrun('ERROR: bulk_aerosol_properties_mod%amcube not yet implemented') | ||
| amcube = -huge(1._r8) |
There was a problem hiding this comment.
It seems to me that the need to look for workarounds like this is a sign that something is wrong with the abstract aerosol class structure. In this case, the abstract properties class is requiring information that is not present in all aerosol models.
In a perfect world, all code that used %amcube (and the other problematic methods below) would reside in the appropriate subclass and all uses would be restricted to that subclass (which could still be a superclass of final models if a set of aerosol models shared a property such as amcube).
I don't see CAM (or CAM-SIMA) getting ready for such a clean class hierarchy soon so in the interim, I think it would be best if the abstract base class (aerosol_properties) provided a default, overridable method to produce an invalid value. I am happy to present this change as a PR to this PR branch so you can evaluate it.
Speaking of an invalid value, what happens if <bulk_aero_prop_obj>%amcube gets called? The code where it is called has no value checks.
modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90
modified: src/chemistry/aerosol/aerosol_properties_mod.F90 modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/carma_aerosol_properties_mod.F90 modified: src/chemistry/aerosol/hygro_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/insoluble_aerosol_optics_mod.F90 modified: src/chemistry/aerosol/modal_aerosol_properties_mod.F90 modified: src/physics/cam/aerosol_optics_cam.F90
|
@gold2718 - Do you have any further feedback, or are you okay with us proceeding with tagging this? |
@cacraigucar, I do have some comments. I will turn in what I have by the end of the day even if it is partial. |
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
First, it is nice to see more of the essential aerosol interface needs being met (e.g., with more of the optics defined in the objects). Thanks for all the work on this.
However, I feel this PR takes the aerosol objects in a direction that makes them more CAM specific and which shuts out other model types.
I think that for now, a lot of the problems can be addressed by having the models identify themselves instead of trying to guess by looking at particular properties. See the suggestion in aerosol_properties_mod.F90.
| procedure(aero_resuspension_resize), deferred :: resuspension_resize | ||
| procedure(aero_rebin_bulk_fluxes), deferred :: rebin_bulk_fluxes | ||
| procedure(aero_hydrophilic), deferred :: hydrophilic | ||
| procedure :: is_bulk |
There was a problem hiding this comment.
It would be really nice if this could instead be a general term or if terms could be added for other types. One suggestion is:
| procedure :: is_bulk | |
| procedure ::aerosol_is |
aerosol_is could function similarly to how dycore_is is used today. This could then be used to identify the bulk model but also could replace current model tests such as (nmodes>0) (currently used to identify MAM but collides with our modal and sectional models) or (nbins>0) (currently used to identify CARMA but also collides with our sectional model).
I'm happy to submit this as an independent PR or as a PR to this PR branch.
There was a problem hiding this comment.
@gold2718 I like this suggestion. Since you offered, please submit a PR to this branch with the changes you have in mind. Thanks.
| !------------------------------------------------------------------------ | ||
| subroutine aero_props_get(self, bin_ndx, species_ndx, list_ndx, density, hygro, & | ||
| spectype, specname, specmorph, refindex_sw, refindex_lw) | ||
| spectype, specname, specmorph, refindex_sw, refindex_lw) |
There was a problem hiding this comment.
I still think each of these properties should be a separate method which would simplify use and maintenance but I can pitch this as an independent PR later if that sounds plausible.
There was a problem hiding this comment.
Implementing this in a separate PR sounds reasonable to me.
There was a problem hiding this comment.
I agree. It will be nice to see it as a clean change to see if it really helps. Shall I open an issue for this (and maybe for optics_params?
| fa(icol,ilev,iwav) = fa(icol,ilev,iwav) + dopaer(icol)*palb(icol)*pasm(icol)*pasm(icol) | ||
|
|
||
| call update_diags | ||
| if (.not.aeroprops%is_bulk()) then |
There was a problem hiding this comment.
Is this somehow generally true for bulk aerosol models or is it specific to the bulk model currently in CAM? This is another place (and below) where an aerosol_is interface could be both more specific and more informative.
There was a problem hiding this comment.
This is specific to how BAM is currently implemented in CAM. With some work we might be able to remove the conditional.
| character(len=32) :: sw_fa_name(nswbands) | ||
| character(len=32) :: lw_ta_name(nlwbands) | ||
|
|
||
| logical :: bam_debug = .false. |
There was a problem hiding this comment.
Is this logical and code that uses it below still needed?
|
@gold2718: FYI - Since you are actively contributing change requests to this PR (thanks!), I will depend on your review approval before I advance this into the tagging phase. Once @fvitt makes the requested changes (or says why he does not want to do so), I will suggest he ask for re-review, so you are aware that he thinks he's done. |
…tatus modified: src/chemistry/aerosol/bulk_aerosol_properties_mod.F90 modified: src/physics/cam/aer_vis_diag_mod.F90
|
At a planning meeting today, this PR was listed as a roadblock for some high priority development. We would like to bring this into CAM later this week, early next week. I would suggest we open issue(s) for significant changes and try to move this PR forward. |
Is it okay for me to open issues for these items? Assuming my
@cacraigucar, are any or all of these okay as new CAM issues? |
|
Hi @gold2718,
I like this idea, but I am concerned with potentially unnecessary copying if the aerosol model does not need this distinction. In general I think a discussion of who holds the 'state' copy of mixing ratios for chemistry and aerosol species will be helpful. If no different internal state is needed then other modules would just point to that copy, otherwise create/manage their own internal state. It would be helpful to have a small proof of concept for discussion. |
|
@gold2718 - yes please open separate issues on all of your points. It would be great if you can get them opened in the next day or so, so that @jimmielin has time to give some initial thoughts before he goes on PTO for a couple of weeks. Also, I should point out that there are funding grants which require that BAM and MAM be in CAM-SIMA in the "near" future, so we locally may take some shortcuts for the short-term and retrofit when things are fully developed. With this in mind, we would like to finalize "soon" the interfaces between chemistry and physics. The interfaces between aerosols and chemistry can be finalized at a later date as they are not required for our immediate development purposes (and those aerosol interfaces will be developed by the ACOM group) @jimmielin @fvitt @nusbaume - Please chime in if I misspoke, as you folks are down in the trenches with this |
4 participants
For the purpose of aerosol optical properties input into radiative transfer, aerosol interfaces have been extended for bulk aerosols.
closes #1400
Roundoff level answer changing.