Develop

[drnimbusrain]

This PR addresses issue #23 , and reorganizes the canopy_driver to be more streamlined and easier for future development. Numerous new routines are added to handle the different main components of the Canopy-App, and has removed significant amounts of code that are not needed and at times redundant.

[zmoon]

I'll look more later, but I think the subroutines that are in files by themselves ¹ would be better off grouped in a module, like canopy_standalone_mod or something. Or canopy_driver_mod and the main program could be canopy_app (which would be fun given the repo name).

Footnotes

1. by this I meant the ones that were introduced here that were in files that did not contain a module ↩

[drnimbusrain]

Oh, I like it Zach, I can make it like that during your review. I hope we are approaching a better framework with each step. :)

…

On Thu, Oct 6, 2022, 6:12 PM Zachary Moon ***@***.***> wrote: ***@***.**** commented on this pull request. I'll look more later, but I think the subroutines that are in files by themselves would be better off grouped in a module, like canopy_standalone_mod or something. Or canopy_driver_mod and the main program could be canopy_app (which would be fun given the repo name). — Reply to this email directly, view it on GitHub <#30 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGLFYNQZ56NTX44YZ2ES24LWB5FEFANCNFSM6AAAAAAQ66UYZ4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[zmoon]

I think d89f760 was maybe a step too far. I was envisioning that the WAF, phot, etc. routines, which might be called by hypothetical future CCPP or what-have-you driver, could stay separate. But routines that are specific to the standalone driver, could go the in the canopy_driver_mod. I'll ponder this more, though.

[drnimbusrain]

@zmoon OK, added WAF as separate option as discussed. Will now revert and pull out all major canopy options, canopy_wind, canopy_waf, canopy_phot, and canopy_eddy as separate submodules again to maintain modularity.

[drnimbusrain]

@zmoon Does this PR look OK now, or any additional comments? After we merge this PR on the overall framework, we can split off using different branches for other features (including NetCDF support) and new modules.
Thank you so much!

[zmoon]

@drnimbusrain I started a review but didn't finish yet, I'll submit what I have today tomorrow.

[zmoon]

Some various comments, but main one is that all subroutines should be in modules, and imports in the main program should be explicit. I think most of the subroutines currently in their own file could go in the driver mod for now.

This is what the current program and module use connections look like ¹. You can see that it doesn't tell the whole story, due to the above.

graph TD
	canopy_app
	canopy_app --> canopy_files_mod
	canopy_canmet_mod
	canopy_canopts_mod
	canopy_canvars_mod
	canopy_const_mod
	canopy_coord_mod
	canopy_driver_mod
	canopy_driver_mod --> canopy_const_mod
	canopy_driver_mod --> canopy_utils_mod
	canopy_eddy_mod
	canopy_eddy_mod --> canopy_const_mod
	canopy_files_mod
	canopy_phot_mod
	canopy_phot_mod --> canopy_const_mod
	canopy_txt_io_mod
	canopy_txt_io_mod --> canopy_canmet_mod
	canopy_txt_io_mod --> canopy_canopts_mod
	canopy_txt_io_mod --> canopy_canvars_mod
	canopy_txt_io_mod --> canopy_coord_mod
	canopy_utils_mod
	canopy_utils_mod --> canopy_const_mod
	canopy_waf_mod
	canopy_waf_mod --> canopy_const_mod
	canopy_waf_mod --> canopy_utils_mod
	canopy_wind_mod
	canopy_wind_mod --> canopy_const_mod

Loading

Footnotes

1. Using fortdepend -g followed by sed 's/ ->/ -->/g' canopy-app.dot | sed '1 s/^.*/```mermaid\ngraph TD/' | sed '$ s/^.*/```/' ↩

[drnimbusrain]

@zmoon Thank you for the excellent review!

For your main comment, I am a bit confused on how you suggest it should look though. Can you derive a similar flow chart that describes what it should look like, and/or would you mind making those suggested changes in another branch and doing a PR so I can better see how you envision the changes should look? I am somewhat wondering why modules are better here than their own subroutines that house the read, alloc, init, and dealloc steps. It seems this way treats them as major canopy-app model (or any model) components, and keeps it more simplified in the main canopy-app program.

The canopy-driver mod, in my mind, should contain the main driving components that describe the canopy, and should not necessarily contain the read, alloc, init, and dealloc as you suggest should be in that module. Also, I think of these files as main components of the canopy-app model, not necessarily needing to be modules.

The main component options, which I agree should stay modular, are in modules I agree. Maybe some more input onto why this current structure would limit our applications of canopy-app would be helpful too.

Thanks!

[zmoon]

@drnimbusrain I changed the dx calc. git checkout 3ea35e1 if you want to run it and see the comparison. They were same to 6 or 7 sig figs I think. I put abs on the dlon so that it won't be negative, but I also confirmed.

[drnimbusrain]

@zmoon Yes, I like your dx change and have tested it. Can you approve this PR so we can merge now and branch off?