Closed takmichibata closed 5 years ago
Dear Tak -
Thanks for contributing code to COSP. I have two initial requests for changes I hope you can make. I may come up with more as I look deeper into the code.
1) It would be more helpful to users and developers new to COSP to have more informative variable names. "CFFOD" doesn't tell users very much. (It's also not accurate - you could make a "contoured figure" from this data if you wanted but the numbers themselves are not a figure.) I'd be grateful if you could change CFFOD, PDFMAP, and the scale variables PDFMAP_NPHASE, CFODD_NBINX, CFODD_NBINY, to something more informative and related to the variables being accumulated.
2) We provide infrastructure to compute joint histograms: subroutine hist2D() in module MOD_COSP_STATS. It would be better to use this routine where possible instead of your own code for computing joint histograms.
Please let me know what you think about these suggestions.
Dear Robert,
Thank you for suggestions. I agree with you opinion.
According to the first comment, I will carefully change the variable names with regard to CFODD, so as to be consistent with "CFAD" variable names as much as possible. PDFMAP_NPHASE will be also modified for more appropriate variable name.
Secondly, I will also replace the code of computing joint histograms by hist2D().
I would like to update as soon as possible. Thank you for your cooperation.
Regards, Tak
Dear Robert and Dustin -
I have modified some variable names used in the "inline statistics routine for warm-rain", according to the previous comment. For example, "npdfmap" which means total sample number of warm-rain occurrence frequency, was changed to "wr_occfreqntotal". I still use names "CFODD***", but I made some changes so as to be consistent with the existing "CFAD" variable names. I think it will be useful for tracking throughout the codes.
I do not use Hist2D() function here, instead I use INT() intrinsic function. In my view, the computational cost to construct joint histogram will be reduced in this method, because we can determine the point within the dBZe*ICOD dimensions without using do-loop which searches all grid cells one-by-one.
I'd be happy if we call the tool "Inline Diagnostic Driver (IDiD)" as a public nickname, because the IDiD will be expected to extend its scope of application to not only warm-rain (IDiD-WR) but also mixed-phase and ice clouds diagnostics, in near future. I hope the IDiD will be helpful for the CFMIP/COSP community.
Best regards, Tak
Hi Tak,
Sorry for the long delay with my response. Firstly, there was a small update to the COSP2 master branch (fixing a small bug in some new LIDAR diagnostics, which shouldn't impact any of your work), but you should update your forked repository to reflect these changes.
Some specific comments 1) I checked out your code and had some issues building and linking, FYI I'm using the standard intel fortran setup provided for the offline driver. ) Building: In the routine cosp_init(), @line1828 of src/cosp.F90, you are trying to nullify your new output fields in the derived type cospOUT: if ( Lwarmrain ) nullify(cospOUT%cfodd_ntotal, cospOUT%wr_occfreq_ntotal) However, cospOUT is not defined in this routine, so the compiler is squawking during the build. Also, all fields in cospOUT are also set to null() on declaration, so this is unnecessary. With that being said, passing your new logical flag Lwarnrain to cosp_init() is unnecessary. ) Linking: The Makefile provided is setup to build libraries, which are then linked to the offline driver. When building the offline driver I get a linking error, as your new module cosp_diagnostics.F90 is not being linked to libcosp.a. 1a) Similar to the linking issue, your new diagnostics are not included as part of the offline driver output. You should include these new diagnostics in driver/src/cosp2_io.F90. I understand that you are focused on running inline, but additions to COSP should still work in offline mode.
2) Keeping with the current organization, I feel that the contents of cosp_diagnostics.F90 should be in cosp_stats.F90 rather than in it's own file. Currently cosp_stats.F90 contains all of the code used for joint-simulator products, so it is a fitting home for your new routine.
3) This echoes some of what Robert mentioned earlier, but you have one logical flag, Lwarmrain, controlling two diagnostics, which I understand are dependent on one another but they should still be controlled independently. It may be the case that a user only wants to archive one of the fields, maybe this never happens, but in COSP2 we have this functionality for all of the other diagnostics and would like to preserve it going forward.
4) In src/cosp.F90 you compute zlev and delz, one (zlev) of which is just copied over from fields in cosp_config.F90. The other (delz) can also be computed in cosp_config.F90. You then pass these fields into your new routine. Why not just have your routine use these fields directly from cosp_config.F90?
Dustin
Hi Dustin,
I appreciate your code review and helpful feedback.
Firstly, there was a small update to the COSP2 master branch...
Thank you for the notification. These updates are merged into my developed branch. I have modified the codes of warm-rain diagnostics, following to your comments:
Firstly, I apologize that I did not check my codes on the “offline driver” mode. I have confirmed that the updated version works without any issues building/linking errors, which was tested on the intel fortran environment for offline driver. In addition, I have added output description for new diagnostics into driver/src/cosp2_io.F90.
Yes, I agree with you opinion. The cosp_diagnostics.F90 was deleted and merged into cosp_stats.F90 in the updated version.
I prepared two logical flags (Lwr_occfreq and Lcfodd) at driver/run/cosp2_output_nl.txt, which control each diagnostic output independently.
I defined dz at cosp_config.F90, and it is initialized in COSP_INIT at cosp.F90.
I am now checking the codes overall. If you find any problems, please let me know.
Best regards, Tak
@takmichibata I tested these diagnostics offline and everything seems to be working as expected. I will merge your changes onto the master branch and tag it as COSPv2.1.5. With each tag there is the opportunity to provide some additional information on what is included in the update (see the previous releases for some examples: https://github.com/CFMIP/COSPv2.0/releases). I encourage you to do the same for v2.1.5.
Thanks again for your patience with this process.
Cheers, Dustin
The following codes have been modified a little, due to the implementation of IDiD. Please also check the new file "src/cosp_stats_idid.F90", which does main work of the inline warm rain diagnostics. As MIROC does not use NetCDF output format, I do not change I/O code (maybe driver/src/cosp2_test.f90).