Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor of module responsible for reading-in gridded inputs from NetCDF file #85

Merged
merged 25 commits into from
Sep 21, 2023

Conversation

GreyREvenson
Copy link
Contributor

@GreyREvenson GreyREvenson commented Sep 14, 2023

PURPOSE

The purpose of this PR is to refactor the Noah-OM (gridded version) module that handles tasks associated with reading-in gridded model inputs from a NetCDF file. The previous iteration of this module assumed that each gridded input variable (e.g., soils, slope, etc.) would be provided in a separate input NetCDF file and had one subroutine to read each specific file. Instead, this PR assumes that all gridded input variables are provided in a single NetCDF file and implements a generic type-bound ReadVar subroutine that will be called for all gridded input variables.

ADDITIONS

The NetCDFVarsType module was added and contains the following type definitions:

  • netcdfvars_type, which is a container for all gridded input variables.
  • netcdf2dvar_type, which is meant as a generic type with a name attribute
  • netcdf2dvarINT_type, which extends the netcdf2dvar_type and has the attribute data, which is an allocatable array of integer values
  • netcdf2dvarREAL_type, which extends the netcdf2dvar_type and has the attribute data, which is an allocatable array of real values
  • netcdf2dmetadata_type, which is meant to hold all metadata associated with the NetCDF input file and all variables that are meant to be read from the file. A single instance of this type is given as an attribute for netcdfvars_type

The netcdfvars_type has one instance of netcdf2dvar_type (either netcdf2dvarINT_type or netcdf2dvarREAL_type) for each gridded input variable (e.g., soils, slope). Each instance of netcdf2dvar_type is given as an argument when calling netcdfvars_type%ReadVar. ReadVar is overloaded based on the argument instance of netcdf2dvar_type being either netcdf2dvarINT_type or netcdf2dvarREAL_type.

This PR adds a new dummy NetCDF input file (/data/netcdf_input.nc) to the repo. This PR also adds a py script (/test/analysis/create_dummy_netcdf_singlefile.py) to create the dummy NetCDF file. The dummy NetCDF file depicts cell (1,1) as having input values equivalent to the values within namelist.input of the main branch of this repo -- thus cell (1,1) should have the same model outputs as the column model (assuming use of same namelist.inpu file).

CHANGES

This PR assumes that all gridded inputs will be given to the model in a single NetCDF file and the path/name of this file must be provided via the namelist.input file. The names of all required variables, dimensions, and attributes within the single NetCDF file are planned to be read-into the model via namelist.input but they are currently hardcoded until I get feedback regarding this PR.

The netcdfvars_type replaces the now removed gridinfo_type and is therefore passed to the Init and/or the InitTransfer subroutines for the gridded derived type members of noahowpgrid_type (i.e. 'the model') -- either to provide access to n_x and n_y for array allocation purposes or to transfer read-in gridded variables.

TESTING

I executed test/analysis/compare_outputnc.py and got this output. As expected, grid cell (1,1) passed all tests while the other grid cells failed for one or more tests (inputs for cell (1,1) are the same as what's listed in namelist.input in the main branch of this repo).

TODO

Read-in variable/dimension/attribute names via namelist.input

@GreyREvenson GreyREvenson marked this pull request as draft September 14, 2023 19:14
@GreyREvenson
Copy link
Contributor Author

@hellkite500, would you be able to provide a critique of this refactor of the NetCDF read-in functionality? Does this jive with what you were aiming for? Things to improve upon?

Note that I did not address your desire to be able to deallocate/reallocate the model's allocatable arrays, which I thought we'd address in a separate PR.

@GreyREvenson GreyREvenson marked this pull request as ready for review September 19, 2023 19:10
@GreyREvenson
Copy link
Contributor Author

@hellkite500, I know you're a particularly busy person so I requested a review from @nmizukami too. Appreciate you having a look!

Copy link
Contributor

@nmizukami nmizukami left a comment

Choose a reason for hiding this comment

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

One comment.. I am not sure if this really works, but since you extended type netcdfvars_type to integer and real, you can use input argument as class for readVar (now two routines for each type - readVarInt and readVarReal).

This will need to be tested to make sure my suggestion works or not.

I will keep looking the rest of codes and may have more comments.


end subroutine ReadSpatial

subroutine ReadVarINT(this,netcdf2dvarINT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder this and ReadVarReal can be combined like this

subroutine ReadVar (this, netcdf2dVar)
    class(netcdfvars_type),    intent(in)    :: this
    class(netcdf2dvar_type), intent(inout) :: netcdf2dvar
   ....
   ...

Then, replace generic procedure in Line 72 with regular public procedure (no need for line 70 and 71). Idea is to remove duplicated routines.

This talks about Polymorphism. But I don't know if you need select type, type is ....
https://www.pgroup.com/blogs/posts/f03-oop-part2.htm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @nmizukami! I'll give this a try today and report back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmizukami This worked nicely and I pushed an update for you to look over. Thanks for the suggestion.

I did need to implement a few select type blocks as I found that I could not access the data arrays outside these blocks.

@nmizukami
Copy link
Contributor

One another comment was about save statement in each F90 defining type. Wondering if there is need for this? These F90s are to define the type (nothing is instantiated, and nothing to save the values?)

@GreyREvenson
Copy link
Contributor Author

@nmizukami I'll also test your suggestion on the save statements. I'll put more thought into that.

@GreyREvenson
Copy link
Contributor Author

One another comment was about save statement in each F90 defining type. Wondering if there is need for this? These F90s are to define the type (nothing is instantiated, and nothing to save the values?)

You were right, @nmizukami : the save statement was not needed. I removed the save statement from the NetCDFVarsType module, recompiled, re-executed the model, and then re-executed /test/analysis/compare_outputnc.py with expected results. I pushed this change.

I'll also test removal off all other save statements in the type definition modules and put those in a separate PR for you to review.

@nmizukami
Copy link
Contributor

One another comment was about save statement in each F90 defining type. Wondering if there is need for this? These F90s are to define the type (nothing is instantiated, and nothing to save the values?)

You were right, @nmizukami : the save statement was not needed. I removed the save statement from the NetCDFVarsType module, recompiled, re-executed the model, and then re-executed /test/analysis/compare_outputnc.py with expected results. I pushed this change.

I'll also test removal off all other save statements in the type definition modules and put those in a separate PR for you to review.

Hi @GreyEvenson-NOAA , Yes, I agree with separate PR for removing save! thanks for checking this.

Copy link
Contributor

@nmizukami nmizukami left a comment

Choose a reason for hiding this comment

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

Hi @GreyEvenson-NOAA, I don't have any other comments so I will check Approve now.

Copy link
Member

@hellkite500 hellkite500 left a comment

Choose a reason for hiding this comment

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

Finally got to look through this PR, and I think this definitely going the right direction. Nice work on the NetCDF encapsulation!

The next steps are to think about how to start decoupling the grid/spatial from the netcdf dependency. That is, the ability to initialize a gridded model domain from a specified grid spec, and then validate the grid requested is a valid subset of the available netcdf grid data.

That can definitely be a follow on to this work though. As is, this is quite useful in extending the support of available gridded netcdf inputs without having to duplicate a lot of code!

@GreyREvenson
Copy link
Contributor Author

Appreciate your time and input on this, @hellkite500 and @nmizukami. I'll merge this PR now.

@nmizukami: Depending on your availability, I may send one PR regarding the save statements in the type-definition modules and a second PR to read-in the NetCDF variable/dimension/attribute names from namelist.input.

@hellkite500: Sounds good regarding the decoupling. @SnowHydrology explained this as requirement for parallelization, though I admittedly may have misunderstood. Will be cool to talk more on this.

@GreyREvenson GreyREvenson merged commit e789a95 into NOAA-OWP:noah_om_grid Sep 21, 2023
@nmizukami
Copy link
Contributor

Hi @GreyEvenson-NOAA , yes, I am happy to review the next PRs too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants