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

Code clean-up that I noticed while putting together the tutorial #139

Open
mnlevy1981 opened this issue Feb 24, 2017 · 3 comments
Open

Code clean-up that I noticed while putting together the tutorial #139

mnlevy1981 opened this issue Feb 24, 2017 · 3 comments

Comments

@mnlevy1981
Copy link
Collaborator

Lots of small (and some not-so-small) things that irked me while cutting / pasting code into the tutorial / user guide / dev guide. Some of these are mentioned in other issue tickets as well, I'll cross-list when possible.

  1. Most instances of surface_forcing (set_surface_forcing(), num_surface_forcing_elements(), etc) should be changed to surface_fluxes.
  2. Similarly, most instances of interior_forcing should be changed to interior_tendencies
  3. Consistency among different pairs of marbl_single_XYZ_type and marbl_XYZs_type
    1. Adding new object should always use reallocatable instead of counting, allocating, and then actually adding (diagnostics may want to allocate more than 1 at a time to cut down on number of deallocate statements)
    2. Some reallocatable arrays copy twice (into temp array, then out of temp array) while others use pointers to avoid the second copy (copy to new array, deallocate original array and point to new)
    3. Some use %add() to add new member, others use %add_diag() or %add_sfo()
  4. Re-organize definition of marbl_interface_class to list elements in 4 categories from the tutorial
    1. Private Data (items that need to be thread safe)
    2. intent(in)
    3. intent(out)
    4. methods
  5. Clean up setup (see Create marbl_interface%setup for GCMs with MARBL namelist? #138 )
  6. Organize methods in marbl_interface_class so that "big 6" (config, init, complete_config_and_init, set_surface_forcing, set_interior_forcing, shutdown) are listed first (and in that order)
  7. Have shutdown() deallocate memory or call deconstructors
    1. Including timers - force users to use extract_timing to get timer data

I may have forgotten an item or two - I'll add to this list if I think of anything else while running through the tutorial.

@mnlevy1981 mnlevy1981 added this to the MARBL1.0.0 milestone Feb 24, 2017
@mnlevy1981
Copy link
Collaborator Author

Rename marbl_instance%surface_vals to marbl_instance%tracer_surface_vals? Want something in variable name that specifies that surface_vals relates to tracers.

@mnlevy1981
Copy link
Collaborator Author

Regarding item 3 in the first list -- @klindsay28 pointed out that the new timers don't follow this convention (we have marbl_single_timer_type and marbl_internal_timers_type with marbl_timers_type used for the timing summary reported to the GCM). I think we want to rename marbl_internal_timers_type -> marbl_timers_type (consistent with the single_XYZ_type / XYZs_type distinction) and rename the current marbl_timers_type -> marbl_timer_summary_type.

@mnlevy1981
Copy link
Collaborator Author

On the topic of reallocatable arrays: talking with @klindsay28 during a code review yesterday, two things to explore came up:

  1. Switching to an opaque datatype, where the data itself is private and we need to define get_*() routines to access
  2. Using extensible classes to define a generic marbl_reallocatable_type (or similar) that allows us to use the same %add() member.

Neither of these points were sketched out in any detail, and it's possible that one (or both) turns out to be impractical when we try to actually implement.

An example of the opaque datatype: the POP driver needs to know how many diagnostics are being stored in marbl_instance%surface_forcing_diags and marbl_instance%interior_forcing_diags; rather than using marbl_instance%surface_forcing_diags%diags_cnt or size(marbl_instance%surface_forcing_diags%diags), we could create marbl_instance%get_surface_diag_cnt() or marbl_instance%surface_forcing_diags%get_diag_cnt().

The main benefit is that when users ask "how many diagnostics are being returned?" we can point them to a single function that will tell them instead of needing to go into details about the marbl_diagnostic_type.

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

No branches or pull requests

1 participant