Add delft3D to datacube converter by yukms · Pull Request #1 · sandpiper-toolchain/render

yukms · 2026-03-11T23:08:39Z

No description provided.

Copilot

Pull request overview

Adds a Delft3D (TRIM NetCDF) → Sandplover DataCube conversion utility under render/, along with a companion notebook demonstrating usage.

Changes:

Add Delft3DConverter to extract key Delft3D variables and write a NetCDF-formatted cube.
Add an example Jupyter notebook documenting and running the conversion workflow.
Add render/ documentation + license file (plus unintended .DS_Store artifacts).

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`render/Delft3D_to_datacube/delft3d_converter.py`	Implements Delft3D → `DataCube` conversion and NetCDF export.
`render/Delft3D_to_datacube/create_datacube_from_Delft3D.ipynb`	Demonstrates running the converter and describes produced variables.
`render/README.md`	Documents the purpose of the `render/` tools directory.
`render/LICENSE`	Adds a GPLv3 license copy within `render/`.
`render/.DS_Store`	Unintended macOS metadata file added to repo.
`.DS_Store`	Unintended macOS metadata file added to repo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T23:12:45Z

+        # Delete existing file if needed
+        if os.path.exists(output_path) and overwrite:
+            os.remove(output_path)


overwrite=False currently doesn’t prevent overwriting an existing file: if output_path exists and overwrite is false, the code falls through and later to_netcdf will still write with the default overwrite mode. Add an explicit guard (raise if the file exists) or pass a non-overwriting mode (e.g., mode='w-') when overwrite is false.

Suggested change

# Delete existing file if needed

if os.path.exists(output_path) and overwrite:

os.remove(output_path)

# Handle existing file according to overwrite flag

if os.path.exists(output_path):

if overwrite:

os.remove(output_path)

else:

raise FileExistsError(

f"Output file '{output_path}' already exists and overwrite is set to False."

)

I agree with the AI that this should probably raise an error rather than allow the user to continue.

Copilot · 2026-03-11T23:12:45Z

+    "- **sand_frac**: Sand fraction (1 - MUDFRAC)\n",
+    "\n",
+    "**Note:** x and y dimensions are swapped in the output file.\n",
+    "**Note:** To customize variables, edit 'delft3d_converter.py"


This note line is missing its closing quote/backtick/period, so it renders as an unterminated string in the markdown cell. Please close the filename reference (prefer backticks, e.g., delft3d_converter.py) and end the sentence properly.

Suggested change

"**Note:** To customize variables, edit 'delft3d_converter.py"

"**Note:** To customize variables, edit `delft3d_converter.py`.\n"

Copilot · 2026-03-11T23:12:46Z

+    "# Set file paths: change to NETCDF file location\n",
+    "trim_file = '/Users/minsik/Desktop/NETCDFoutput_TEST/trim-DV_run.nc'\n",
+    "output_file = '/Users/minsik/Desktop/NETCDFoutput_TEST/sandplover_cube.nc'\n",
+    "\n",
+    "# Run conversion\n",
+    "converter = Delft3DConverter(trim_file).convert(output_file, show_stats=True)"


The example uses hard-coded absolute paths under /Users/..., which won’t work for other users or CI. Consider using a relative path, a configurable parameter (env var/CLI arg), or a placeholder string (e.g., '/path/to/trim.nc') in the notebook example.

I think the second option would be the better repair. If you make them placeholder strings, that will be better.

Copilot · 2026-03-11T23:12:46Z

+
+    def load_dataset(self):
+        """Load the Delft3D NetCDF dataset."""
+        self.trim_ds = xr.open_dataset(self.trim_file_path)


xr.open_dataset is currently called without an explicit decode_timedelta setting. The included notebook run shows FutureWarnings about timedelta decoding; consider passing decode_timedelta=True/False explicitly (or otherwise handling these variables) to keep behavior stable across xarray versions.

Suggested change

self.trim_ds = xr.open_dataset(self.trim_file_path)

self.trim_ds = xr.open_dataset(self.trim_file_path, decode_timedelta=True)

Up to you if you want to follow the AI

Copilot · 2026-03-11T23:12:46Z

+    "- **eta**: Bed elevation (DPS)\n",
+    "- **water_depth**: Water depth (S1)\n",


The variable descriptions in the notebook don’t match the converter logic: the code computes eta = S1 - DPS and stores water_depth from DPS, but the notebook lists eta as “(DPS)” and water_depth as “(S1)”. Please update these bullets to reflect the actual calculations/sources.

Suggested change

"- **eta**: Bed elevation (DPS)\n",

"- **water_depth**: Water depth (S1)\n",

"- **eta**: Water depth (S1 - DPS)\n",

"- **water_depth**: Bed elevation (DPS)\n",

Worth checking this. You know Delft better than me, but it looks like the AI might be confused, since I suppose S1 might be the water surface elevation and the depth is probably DPS, giving $\eta$ as the difference. So it might be worth clarifying.

Copilot · 2026-03-11T23:12:46Z

+   "outputs": [
+    {
+     "name": "stderr",
+     "output_type": "stream",
+     "text": [
+      "/Users/minsik/Desktop/NETCDFoutput_TEST/delft3d_converter.py:37: FutureWarning: In a future version, xarray will not decode the variable 'MORFT' into a timedelta64 dtype based on the presence of a timedelta-like 'units' attribute by default. Instead it will rely on the presence of a timedelta64 'dtype' attribute, which is now xarray's default way of encoding timedelta64 values.\n",
+      "To continue decoding into a timedelta64 dtype, either set `decode_timedelta=True` when opening this dataset, or add the attribute `dtype='timedelta64[ns]'` to this variable on disk.\n",
+      "To opt-in to future behavior, set `decode_timedelta=False`.\n",
+      "  self.trim_ds = xr.open_dataset(self.trim_file_path)\n",


The notebook is committed with execution outputs that include local filesystem paths (e.g., /Users/...) and warning logs. Please clear all cell outputs before committing (and consider keeping example output minimal) to avoid leaking local environment details and to reduce noisy diffs.

I also agree with the AI about this. It's best practice to clear output cells before committing.

ericbarefoot

So, no issues with the class itself or the method. Looks good. The AI review actually caught a few things that could be improved. I think the main thing that would help is to have a more verbose notebook. When you are deleting the files, you could also reasonably ask CoPilot to make a more verbose description of how to get a file out of delft and how to use it. Then when you make the next notebook that works with the .dat files, you can use the same text and formulae, but with the .dat file.

ericbarefoot · 2026-03-12T15:58:32Z

+        # Delete existing file if needed
+        if os.path.exists(output_path) and overwrite:
+            os.remove(output_path)


I agree with the AI that this should probably raise an error rather than allow the user to continue.

ericbarefoot · 2026-03-12T16:15:23Z

+    "# Set file paths: change to NETCDF file location\n",
+    "trim_file = '/Users/minsik/Desktop/NETCDFoutput_TEST/trim-DV_run.nc'\n",
+    "output_file = '/Users/minsik/Desktop/NETCDFoutput_TEST/sandplover_cube.nc'\n",
+    "\n",
+    "# Run conversion\n",
+    "converter = Delft3DConverter(trim_file).convert(output_file, show_stats=True)"


I think the second option would be the better repair. If you make them placeholder strings, that will be better.

ericbarefoot · 2026-03-12T16:16:36Z

+   "outputs": [
+    {
+     "name": "stderr",
+     "output_type": "stream",
+     "text": [
+      "/Users/minsik/Desktop/NETCDFoutput_TEST/delft3d_converter.py:37: FutureWarning: In a future version, xarray will not decode the variable 'MORFT' into a timedelta64 dtype based on the presence of a timedelta-like 'units' attribute by default. Instead it will rely on the presence of a timedelta64 'dtype' attribute, which is now xarray's default way of encoding timedelta64 values.\n",
+      "To continue decoding into a timedelta64 dtype, either set `decode_timedelta=True` when opening this dataset, or add the attribute `dtype='timedelta64[ns]'` to this variable on disk.\n",
+      "To opt-in to future behavior, set `decode_timedelta=False`.\n",
+      "  self.trim_ds = xr.open_dataset(self.trim_file_path)\n",


I also agree with the AI about this. It's best practice to clear output cells before committing.

ericbarefoot · 2026-03-12T16:19:05Z

+    "- **eta**: Bed elevation (DPS)\n",
+    "- **water_depth**: Water depth (S1)\n",


Worth checking this. You know Delft better than me, but it looks like the AI might be confused, since I suppose S1 might be the water surface elevation and the depth is probably DPS, giving $\eta$ as the difference. So it might be worth clarifying.

ericbarefoot · 2026-03-12T16:21:01Z

Can you remove this file from the commit?

ericbarefoot · 2026-03-12T16:26:07Z

Can you also remove the .DS_Store files from the commit? These are just mac finder placeholder files. I will make a .gitignore for this repo that will propagate back down to you, but for the moment it would be better if these were just removed from the PR entirely.

ericbarefoot · 2026-03-12T16:26:35Z

Also remove this one please.

ericbarefoot · 2026-03-12T16:28:05Z

I think it is bad practice to include the actual data in the repository, since that will end up being cumbersome. for now, let's just make this PR without the data, and then I will suggest a change that will make this a little more lightweight. (it will involve pooch)

ericbarefoot · 2026-03-12T16:29:48Z

I also don't think we need to overwrite the readme. Can you remove this one too?

ericbarefoot

So, no issues with the file or the method. There are few things that I want to see changed before merging.

remove data files and the mac-generated .DS_Store files.
add more descriptive text and explanation to the notebook. Copilot will probably do a good job of this.
most of the AI-generated suggestions are reasonable things to change. I have highlighted a few places where it might have been mistaken.

- Remove .DS_Store, data files (.nc), LICENSE, README.md from tracking - Add .gitignore to exclude .DS_Store, LICENSE, README.md, *.nc, __pycache__, sandplover_analysis.ipynb - Clear all cell outputs from notebook (removes local paths from committed output) - Replace hardcoded filenames with placeholder paths in notebook - Expand notebook markdown with variable table, prerequisites, and step-by-step instructions for adding optional variables Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Move transpose from save_datacube() to extract_variables() so DataCube is created in correct (time, x=227, y=302) order from the start - Fix coord assignments in save_datacube(): dim1→x, dim2→y - Fix misleading comments on coord assignments - Clear all cell outputs and replace hardcoded filenames with placeholder paths in notebook - Expand notebook markdown with variable table, prerequisites, and step-by-step instructions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

김민식 and others added 3 commits March 11, 2026 16:05

Add Delft3D_to_datacube folder

d40d193

Add Delft3D_to_datacube folder with notebook and converter

dcc91cd

Merge branch 'sandpiper-toolchain:main' into main

6fca7dd

Copilot AI review requested due to automatic review settings March 11, 2026 23:08

Copilot started reviewing on behalf of yukms March 11, 2026 23:09 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Add [trim-Test.nc] and update converter

3f9ca0d

ericbarefoot self-requested a review March 12, 2026 15:47

ericbarefoot self-assigned this Mar 12, 2026

ericbarefoot requested changes Mar 12, 2026

View reviewed changes

김민식 and others added 4 commits March 12, 2026 11:27

Add nc/dat converters and ignore Delft3D trim input files

3ec5825

Remove legacy Delft3D_to_datacube folder

3dd2531

-        # Delete existing file if needed
-        if os.path.exists(output_path) and overwrite:
-            os.remove(output_path)
+        # Handle existing file according to overwrite flag
+        if os.path.exists(output_path):
+            if overwrite:
+                os.remove(output_path)
+            else:
+                raise FileExistsError(
+                    f"Output file '{output_path}' already exists and overwrite is set to False."
+                )

	"Note: To customize variables, edit 'delft3d_converter.py"
	"Note: To customize variables, edit `delft3d_converter.py`.\n"

	self.trim_ds = xr.open_dataset(self.trim_file_path)
	self.trim_ds = xr.open_dataset(self.trim_file_path, decode_timedelta=True)

		"- eta: Bed elevation (DPS)\n",
		"- water_depth: Water depth (S1)\n",

Conversation

yukms commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericbarefoot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericbarefoot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants