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

+add h to drifters interface #408

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

cspencerjones
Copy link

This commit brings the drifters interface up-to-date with the current version of the drifters package, which requires h (layer thickness) to calculate the vertical movement of particles. The interfaces in the code and in config_src/external are updated to pass this information to the drifters package.

This commit brings the drifters interface up-to-date with the current version of the drifters package, which requires h (layer thickness) to calculate the vertical movement of particles. The interfaces in the code and in config_src/external are updated to pass this information to the drifters package.
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #408 (2db108b) into dev/gfdl (147ddf1) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 2db108b differs from pull request most recent head 1b1845a. Consider uploading reports for the commit 1b1845a to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #408      +/-   ##
============================================
- Coverage     38.15%   38.15%   -0.01%     
============================================
  Files           269      269              
  Lines         76667    76675       +8     
  Branches      14103    14105       +2     
============================================
  Hits          29254    29254              
- Misses        42146    42152       +6     
- Partials       5267     5269       +2     
Files Changed Coverage Δ
config_src/external/drifters/MOM_particles.F90 0.00% <0.00%> (ø)
src/core/MOM.F90 50.97% <0.00%> (-0.13%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@MJHarrison-GFDL MJHarrison-GFDL left a comment

Choose a reason for hiding this comment

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

The subroutine argument documentation needs unit and rescaling information, e.g. "layer thickness [H ~> m or kg m-2]" and "time [T ~> s]" .

@cspencerjones
Copy link
Author

Thanks - is this ok? Or were you looking for something more?

Copy link

@MJHarrison-GFDL MJHarrison-GFDL left a comment

Choose a reason for hiding this comment

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

Hi Spencer! You can look at other routines to template this. Layer thickness are denoted [H ~> m or kg m-2] , temperature is [C ~> degC], salt is [S ~> ppt], time is [T ~> s]. Sorry to be a nuisance here.

@marshallward
Copy link
Member

marshallward commented Jul 20, 2023

Some other style comments:

  • Indentation should be 2-space. I see many instances of 1-space, 3-space and 4-space. e.g.

        if (CS%use_particles) then
            call particles_to_z_space(CS%particles,h)
        endif
    
      if (CS%use_particles) then
         call particles_to_k_space(CS%particles,h)
      endif
    
  • Commas should usually be followed by a space, e.g.

    subroutine particles_init(parts, Grid, Time, dt, u, v,h)
    
    use MOM_particles_mod,         only : particles_to_k_space,particles_to_z_space
    

    (Array indexing is an exception)

More than you could want to know at the style guide. As Matt said, sorry to be a nuisance.

@cspencerjones
Copy link
Author

Thanks @marshallward and @MJHarrison-GFDL ! I hope I caught everything. Now I'll know better what to look for next time.

Copy link

@MJHarrison-GFDL MJHarrison-GFDL left a comment

Choose a reason for hiding this comment

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

Thank you for making those changes Spencer. This PR looks good .

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/jobs/106995 ✔️

@marshallward marshallward merged commit d5ba107 into NOAA-GFDL:dev/gfdl Jul 25, 2023
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