add log_diag_field_info to dmUpdate#1090
Conversation
thomas-robinson
left a comment
thomas-robinson
left a comment
There was a problem hiding this comment.
By trying to make the code more easy to understand by renaming the variable, I think you added complexity that requires mental gymnastics and unnecessary code execution further down the call stack.
| & units=units, missing_value=missing_value, var_range=range, mask_variant=mask_variant, & | ||
| & standard_name=standard_name, verbose=verbose, do_not_log=do_not_log, err_msg=err_msg, & | ||
| & interp_method=interp_method, tile_count=tile_count, area=area, volume=volume, realm=realm) | ||
| & standard_name=standard_name, verbose=verbose, do_not_log=.not.(do_diag_field_log.AND.allow_log), & |
There was a problem hiding this comment.
This is adding unneeded complexity. do_not_log is optional, so if it's not present, then it won't be passed on because it's optional in the next routine (register_diag_field_array) as well.
You should move this logic to where it's actually used instead of now actually passing something to do_not_log.
There was a problem hiding this comment.
The logic basically prints the log if either the nml flag or do_not_log is set to output , I added it here cause I needed the nml flag and this was the last place where it would be accessible.
I'm gonna change it around anyway so I might take out the do_not_log from the object's register routine if it ends up being useless (which seems likely)
| & long_name=long_name, units=units, missing_value=missing_value, range=range, mask_variant=mask_variant, & | ||
| & standard_name=standard_name, dynamic=DYNAMIC, do_not_log=do_not_log, interp_method=interp_method,& | ||
| & tile_count=tile_count, area=area, volume=volume, realm=realm) | ||
| & standard_name=standard_name, dynamic=DYNAMIC, do_not_log=.not.(do_diag_field_log.AND.allow_log), & |
There was a problem hiding this comment.
This is the same comment as above.
| INTEGER, OPTIONAL, INTENT(IN) :: diag_model_subset | ||
| INTEGER, DIMENSION(6), OPTIONAL, INTENT(IN) :: time_init !< Model time diag_manager initialized | ||
| CHARACTER(len=*), INTENT(out), OPTIONAL :: err_msg | ||
| CHARACTER(len=1), INTENT(in), OPTIONAL :: logfile_separator !< character to use as a csv-style data field separator |
There was a problem hiding this comment.
This could just as easily be a module or namelist variable. Then you don't need to change the interface at all
| !! code uses a do_not_log parameter in the registration calls, | ||
| !! and subsequently calls this subroutine to log field information | ||
| !! under a generic name. | ||
| SUBROUTINE fms_log_field_info(module_name, field_name, axes, long_name, units,& |
There was a problem hiding this comment.
Is this routine used by both old and new? Do we need two?
There was a problem hiding this comment.
This routine is only used by the new. Its pretty similar to the old but some info needed to come from the diag object, also it has the changes for the separator and writes the header instead of doing that in the init.
My thinking was that if theres a new register routine in a separate file it would make sense to add a new log routine too. I think I'll change it though, it will simplifiy the do_not_log logic as well if I just make some changes to the original and use that for both.
|
@rem1776 This kind of fell by the wayside. Is this PR needed anymore? |
Yeah we'll still need some changes to get the routine working with the objects, I'll update this. |
2 participants
Description
adds the log_diag_field_info calls for the modern diag's register routines.
How Has This Been Tested?
intel and gnu on amd
Checklist:
make distcheckpasses