Feature/cdo post ocean format #1875

HenryRWinterbottom · 2023-09-21T18:13:42Z

This PR addresses issue #923. A bash script is provided which updates/adds the necessary netCDF variable metadata attributes such that downstream applications can correctly remap the specified netCDF variables. No special Python libraries or utilities are required. Leveraging the capabilities of NCO allow support across all development and production platforms.

Resolves #923
Refs #1871

Type of change

New feature (adds functionality)

Change characteristics

This is a non-breaking change
This change does not require any documentation updates.

How has this been tested?

Tested using ice and ocean forecast files provided by @NeilBarton-NOAA and @jiandewang and executed on RDHPCS Hera, NOAA CSP AWS, and OSX.

Checklist

Any dependent changes have been merged and published
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing tests pass with my changes
I have made corresponding changes to the documentation if necessary

ush/cdo_post_ocean_format.sh

WalterKolczynski-NOAA

This is good for stand-alone script, but it needs to be connected into the workflow by being called by ocnpost.sh (and remove the NCL scripts being replaced).

WalterKolczynski-NOAA · 2023-09-22T02:04:59Z

parm/post/mom6_update.csv

Why is "update" in the filename? Should this be called mom6_variables.csv instead?

No. The remapping for the MOM6 tripolar to a destination grid projection requires two steps:

The netCDF attributes need to be updated for the specified MOM6 variables to be remapped;

Using the the script introduced in PR CDO based post-processing application #1871 the variables specified in the parm/post/mom6_interp.csv can then be correctly interpolated; this is not required for the CICE forecast output.

ush/cdo_post_ocean_format.sh

WalterKolczynski-NOAA · 2023-09-22T02:29:30Z

ush/cdo_post_ocean_format.sh

+function _strip_whitespace(){
+    local in_string="${1}"
+
+    out_string=$(echo "${in_string}" | $(command -v sed) "s/ //g")


Suggested change

out_string=$(echo "${in_string}" | $(command -v sed) "s/ //g")

out_string=${in_string//[[:space:]]}

I attempted this during development and it was not working as I needed it to.

Hmmm, it worked when I tried a test case with spaces and tabs in a shell. Try again in case this is slightly different than you tried, then if still doesn't work, do this:

Suggested change

out_string=$(echo "${in_string}" | $(command -v sed) "s/ //g")

out_string=$(sed "s/ //g" <<< "${in_string}")

Ironically, I can't get mine to work using sed. It leaves the internal whitespace:

walter@ubuntu:~$ cat test2.bash a=" b c " echo "-->${a//[[:space:]]}<--" echo "-->$(sed 's/ //g' <<< "${a}")<--" echo "-->$(echo "${a}" | sed "s/ //g")<--" walter@ubuntu:~$ bash test2.bash -->bc<-- -->b c<-- -->b c<--

ush/cdo_post_ocean_format.sh

WalterKolczynski-NOAA · 2023-09-22T02:42:12Z

ush/cdo_post_ocean_format.sh

+#   $1 - The comma-delimited string to split.
+#
+# Global Variables:
+#   global_array - An array containing the split elements.


I know we are limited by how bash handles arrays, but I really hate this (setting a random global variable with the result). Would much rather pass a variable name in to be set (this is similar to how generate_com() does things). See associated code suggestions below.

WalterKolczynski-NOAA · 2023-09-22T03:06:08Z

ush/cdo_post_ocean_format.sh

+function _comma_split_string() {
+    local string="${1}"
+
+    local local_array=()
+    global_array=()
+    IFS="," read -ra items <<< "${string}"
+    for item in "${items[@]}"; do
+        local_array+=("${item} ")
+    done
+    for item in "${local_array[@]}"; do
+        IFS=" " read -ra items <<< "${item}"
+        for element in "${items[@]}"; do
+            global_array+=("${element} ")
+        done
+    done
+}


Suggested change

function _comma_split_string() {

local string="${1}"

local local_array=()

global_array=()

IFS="," read -ra items <<< "${string}"

for item in "${items[@]}"; do

local_array+=("${item} ")

done

for item in "${local_array[@]}"; do

IFS=" " read -ra items <<< "${item}"

for element in "${items[@]}"; do

global_array+=("${element} ")

done

done

}

function _comma_split_string() {

local string="${1}"

local var_name="${2}"

# Declare local_array as a reference to the desired array name

declare -n local_array="${var_name}"

IFS="," read -ra items <<< "${string}"

for item in "${items[@]}"; do

local_array+=("${item}")

done

}

Credit to ChatGPT for the declare -n trick. I was having trouble using declare to copy the array after-the-fact, but now we don't have to.

WalterKolczynski-NOAA · 2023-09-22T03:07:05Z

ush/cdo_post_ocean_format.sh

+    _comma_split_string "${coords}"
+    coords="${global_array[@]}"


Suggested change

_comma_split_string "${coords}"

coords="${global_array[@]}"

_comma_split_string "${coords}" coords

WalterKolczynski-NOAA · 2023-09-22T03:22:18Z

ush/cdo_post_ocean_format.sh

+    coords_str="$(echo "${coords}" | $(command -v tr) -s ' ')"
+    ncattr_str="$(echo "${ncattr}" | $(command -v tr) -s ' ')"
+    echo "Adding netCDF attribute ${ncattr_str} values ${coords_str} to variable ${varname} metadata and writing to file ${output_path}"
+    ($(command -v ncatted) -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")


Using command -v is overkill.

Suggested change

($(command -v ncatted) -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")

(ncatted -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")

This was introduced for the following reasons and with respect to my understanding of the best-practices for bash scripts.

Portability: (command -v) is more portable than directly relying on the presence of a specific file or assuming a command is available at a particular location and works across different Unix-like systems. Error Handling: It handles the edge-cases where a command is not found gracefully by checking the result and taking appropriate action, such as displaying an error message or exiting the script. Flexibility: It can be used in combination with other conditional statements to customize the behavior of the script based on whether a command is available or not.

Will leave it up to @aerorahul, but none of our existing scripts use this, and it clutters things up quite a lot once you start using it everywhere.

I removed it.

WalterKolczynski-NOAA · 2023-09-22T03:26:31Z

ush/cdo_post_ocean_format.sh

+    # Get the attributes for the respective variable.
+    varname=$(echo "${line}" | $(command -v awk) '{print $1}')
+    ncattr=$(echo "${line}" | $(command -v awk) '{print $2}')
+    coords=$(echo "${line}" | $(command -v awk) '{print $3}')
+
+    # Update the variable attributes and write the updates to the
+    # specified output file (see `output_path`).
+    ncattr_update "${varname}" "${ncattr}" "${coords}"


Can't this just be:

Suggested change

# Get the attributes for the respective variable.

varname=$(echo "${line}" | $(command -v awk) '{print $1}')

ncattr=$(echo "${line}" | $(command -v awk) '{print $2}')

coords=$(echo "${line}" | $(command -v awk) '{print $3}')

# Update the variable attributes and write the updates to the

# specified output file (see `output_path`).

ncattr_update "${varname}" "${ncattr}" "${coords}"

# shellcheck disable=SC2086

ncattr_update ${line}

(The shellcheck directive is to stifle complaints about the lack of quotation marks.)

This does not guarantee that the string will be parsed properly into the attributes that are required. The awk issue can be eliminated by adding an exception to the shell linter. Any use of awk will fail with the current configuration.

I wasn't worried about the linter so much as just on a crusade to eliminate as many calls to external programs like sed and tr as possible for simple functions. I still don't see the difference, but I trust you. If you do need to use sed, prefer herestrings (<<< ${line}) over echoing through a pipe.

OK, I will give this another look. It's possible that I overlooked something previously.

WalterKolczynski-NOAA · 2023-09-22T03:33:50Z

Is this supposed to replace #1871? Both are still open. (If so, in the future, we'd also prefer updating existing PRs instead of opening new ones.)

ush/remap_prep.sh

+function _strip_whitespace(){
+    local in_string="${1}"
+
+    out_string=$(echo "${in_string}" | $(command -v sed) "s/ //g")


ush/remap_prep.sh

+    local coords="${3}"
+
+    _comma_split_string "${coords}"
+    coords="${global_array[@]}"


ush/remap_prep.sh

+function _strip_whitespace(){
+    local in_string="${1}"
+
+    out_string=$(echo "${in_string}" | sed "s/ //g")


ush/remap_prep.sh

+    coords_str="$(echo "${coords}" | tr -s ' ')"
+    ncattr_str="$(echo "${ncattr}" | tr -s ' ')"
+    echo "Adding netCDF attribute ${ncattr_str} values ${coords_str} to variable ${varname} metadata and writing to file ${output_path}"
+    (ncatted -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")


ush/remap_prep.sh

+# This example will remove all leading, trailing, and internal
+# whitespace from the input string and display the cleaned result.
+function _strip_whitespace(){
+    local in_string="${1}"


ush/remap_prep.sh

+function _strip_whitespace(){
+    local in_string="${1}"
+
+    out_string=$(echo "${in_string}" | sed "s/ //g")


ush/remap_prep.sh

+function _strip_whitespace(){
+    local in_string="${1}"
+
+    out_string=$(echo "${in_string}" | sed "s/ //g")


HenryRWinterbottom · 2023-10-02T15:26:58Z

The reviewed components of the PR have been added/merged into PR #1871. The workflow hooks have not yet been added as we are awaiting feedback from the CMD team as to whether the results generated from #1871 are satisfactory. If so, the respective workflow hooks will be added.

Closing #1875.

henrywinterbottom-wxdev added 2 commits September 21, 2023 10:55

Added script to perform the netCDF variable metadata attribute update.

1c5a6ce

Added parameter file for MOM6 coordinate variable update.

79c4e69

github-advanced-security bot found potential problems Sep 21, 2023

View reviewed changes

HenryRWinterbottom requested review from aerorahul, WalterKolczynski-NOAA, EricSinsky-NOAA and NeilBarton-NOAA September 21, 2023 18:14

HenryRWinterbottom self-assigned this Sep 21, 2023

HenryRWinterbottom added the feature New feature or request label Sep 21, 2023

henrywinterbottom-wxdev added 2 commits September 21, 2023 14:14

Debugging remap.

16769c1

Bug fixes related to bash string expansions.

ec9935f

github-advanced-security bot found potential problems Sep 21, 2023

View reviewed changes

ush/cdo_post_ocean_format.sh Fixed Show fixed Hide fixed

ush/cdo_post_ocean_format.sh Fixed Show fixed Hide fixed

Removed debug notes.

04acac5

WalterKolczynski-NOAA requested changes Sep 22, 2023

View reviewed changes

Renamed script.

d595cf5

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

henrywinterbottom-wxdev added 2 commits September 22, 2023 12:32

Updated documentation block.

5a878de

Removed as requested.

659754d

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

henrywinterbottom-wxdev added 2 commits September 26, 2023 07:51

Added variables necessary for vector rotation and interpolation.

38da95b

Corrections noted in PR #1871 but relevant to PR #1875.

e550fb2

github-advanced-security bot found potential problems Sep 26, 2023

View reviewed changes

HenryRWinterbottom mentioned this pull request Sep 27, 2023

CDO based post-processing application #1871

Closed

7 tasks

Merge branch 'NOAA-EMC:develop' into feature/cdo_post_ocean_format

710c2f6

HenryRWinterbottom closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/cdo post ocean format #1875

Feature/cdo post ocean format #1875

HenryRWinterbottom commented Sep 21, 2023

WalterKolczynski-NOAA left a comment

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023 •

edited

Loading

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023

WalterKolczynski-NOAA Sep 22, 2023

HenryRWinterbottom Sep 22, 2023

WalterKolczynski-NOAA commented Sep 22, 2023

HenryRWinterbottom commented Oct 2, 2023

	out_string=$(echo "${in_string}" \| $(command -v sed) "s/ //g")
	out_string=${in_string//[[:space:]]}

	out_string=$(echo "${in_string}" \| $(command -v sed) "s/ //g")
	out_string=$(sed "s/ //g" <<< "${in_string}")

	_comma_split_string "${coords}"
	coords="${global_array[@]}"
	_comma_split_string "${coords}" coords

	($(command -v ncatted) -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")
	(ncatted -O -a "${ncattr_str}","${varname}",c,c," ${coords_str}" "${output_path}" "${output_path}")

Feature/cdo post ocean format #1875

Feature/cdo post ocean format #1875

Conversation

HenryRWinterbottom commented Sep 21, 2023

Type of change

Change characteristics

How has this been tested?

Checklist

WalterKolczynski-NOAA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenryRWinterbottom Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterKolczynski-NOAA commented Sep 22, 2023

HenryRWinterbottom commented Oct 2, 2023

HenryRWinterbottom Sep 22, 2023 •

edited

Loading