Converting MFP CSV to YAML schedule #111

iuryt · 2025-01-28T18:43:12Z

The file does not contain the time expected for each of the stations, how should we guess this?

Also, how should we organize this code? Should we embed this to the virtualship init command?
e.g. virtualship init --mfp_file ./CoordinatesExport-Filled.xlsx

So far, this is just a script that creates the yaml file. We can delete them after deciding where to implement it.

I am also using the CSV version to avoid installing openpyxl, but we can also install it, no problem.

Closes YAML schedule from MFP CSV #61
Tests added
Re-ran README.md help commands if the CLI has changed

for more information, see https://pre-commit.ci

ammedd · 2025-01-29T07:48:51Z

I'll ask the students to add a column with station time. I hope it will become available in later downloads from the MFP website directly as well. It is included online.

virtualship init --mfp_file ./CoordinatesExport-Filled.xlsx sounds great! maybe changing file for csv or DD
When you download from MFP, you choose Export Coordinates - DD (decimal degrees). There are other options but I would not cater for those at the moment.

iuryt · 2025-01-29T09:46:22Z

@ammedd
Can you give me a version of this file with the times using the template you plan to use?

VeckoTheGecko · 2025-01-29T12:08:21Z

I was discussing with @ammedd , and I think the best thing to do would be to use the CSV to populate as many fields as it can in the schedule, and leave the rest up to the user to populate in the YAML. This saves getting the user to add columns to the CSV (which I think would be more error prone).

I'll be able to give this a proper review on Friday - just have some other items to clear before then :)

iuryt · 2025-01-29T14:19:49Z

scripts/coordinates_to_yaml_output.yaml

+  - instrument: CTD
+    location:
+      latitude: 55.124089
+      longitude: 5.156524
+    time: "2023-01-01 01:00:00"
+  - instrument: DRIFTER
+    location:
+      latitude: 55.124089
+      longitude: 5.156524
+    time: "2023-01-01 01:00:00"


I was discussing with @ammedd , and I think the best thing to do would be to use the CSV to populate as many fields as it can in the schedule, and leave the rest up to the user to populate in the YAML. This saves getting the user to add columns to the CSV (which I think would be more error prone).

I'll be able to give this a proper review on Friday - just have some other items to clear before then :)

But that means that we need to either leave blank for the dates or guess it, right? There are also stations that will have more than one instrument, but it will be hard to guess they are the same if we leave the dates blank. Of course, they have the same location, but if longitudes and latitudes are too close, it might be harder than just adding a column to the CSV. Although I understand that adding a column manually is more prone to error.

VeckoTheGecko · 2025-01-31T09:52:19Z

Okay, I've managed to look through the code. Thanks for picking this up @iuryt ! I definitely see how this will streamline things for users - especially for the waypoints and the bounding box.

But that means that we need to either leave blank for the dates or guess it, right? There are also stations that will have more than one instrument, but it will be hard to guess they are the same if we leave the dates blank. Of course, they have the same location, but if longitudes and latitudes are too close, it might be harder than just adding a column to the CSV. Although I understand that adding a column manually is more prone to error.

Correct, we'll have to leave the dates blank - its just information that is not provided from the MFP export. Though the order of points in the YAML should match the CSV. If the MFP export changes in future, then we can adapt. So yes, it would also be difficult to guess whether stations are the same. Honestly, I think that we should avoid making that guess. Making it clear that virtualship init --from-mfp will only populate some of the fields, and the user will be required to take it from there (e.g., merging waypoints to the same location that were initially spaced a bit further apart in MFP, filling in the dates from the UI of MFP - since they weren't included in the export).

Its not optimal, but given MFP->virtualship isn't necessarily 1to1, and given the limited export, I think that its all we can do. I also think this is the clearer approach from a maintenance POV - being explicit with what is supported, and by not making additional assumptions which may not be right - and hands the control to the user to modify the schedule file.

Also, how should we organize this code? Should we embed this to the virtualship init command?
e.g. virtualship init --mfp_file ./CoordinatesExport-Filled.xlsx

Sounds like a good plan! My vote is for --from-mfp since that is more in line with CLI conventions. We can then add in the help message something like this would be good:

--from-mfp    Partially initialise a project from an exported xlsx or csv file from NIOZ' Marine Facilities Planning tool (specifically the "Export Coordinates > DD" option). User edits are required after initialisation.

I am also using the CSV version to avoid installing openpyxl, but we can also install it, no problem.

Let's install openpyxl

Other notes:

Could we add an error message to users if the CSV headers are not as expected?
- Incorrect headers -> Error: "Found columns [...], but expected columns [...]. Are you sure that you're using the correct export from MFP?"
- Additional headers -> Warning: "Found additional unexpected columns [...]. Manually added columns have no effect. If the MFP export format changed, please submit an issue https://github.com/OceanParcels/virtualship/issues."
It would be nice to have a message at the end that its up to the user to populate the rest of the YAML
Would you be comfortable writing some unit tests for this functionality as well?

Let me know what your thoughts are on all this ^, and if there's anything I can do to help :)

for more information, see https://pre-commit.ci

iuryt · 2025-02-03T17:48:16Z

@ammedd and @VeckoTheGecko

But I tested it, and the current version is working.
I also added some maximum_depth and buffer for longitude and latitude that depend on the instruments.

    # Define maximum depth and buffer for each instrument
    instrument_properties = {
        "CTD": {"depth": 5000, "buffer": 1},
        "DRIFTER": {"depth": 1, "buffer": 5},
        "ARGO_FLOAT": {"depth": 2000, "buffer": 5},
    }

Any comments on this, @erikvansebille ?

Let me know what you think.

I have not added yet

Error message to users if the CSV headers are not as expected
Additional headers -> Warning
Message at the end that its up to the user to populate the rest of the YAML
Unit tests for this functionality

erikvansebille · 2025-02-04T07:04:50Z

Looks goo, but perhaps explain what buffer means in the dictionary above?

VeckoTheGecko

Good progress! Left some review comments. I think there is some duplication of information between the comments and the code. I recommend in general: Don't Write Comments - rewrite the code (really good YouTube channel in general for code style - highly recommend). The only time I write comments is to explain why something was done. E.g., # Keeping this legacy format for backward compatibility with older datasets

There are many ways to make things self documenting. E.g.,

mfp_to_yaml(
            mfp_file, str(path)
        )  # Pass the path to save in the correct directory

could become

mfp_to_yaml(
            mfp_file, save_directory = str(path)
        )

.

It's possible to have clean, readable code with 0 duplication, but also 0 comments ;)

VeckoTheGecko · 2025-02-04T10:04:57Z

src/virtualship/utils.py

Do you think it would be possible to use the Pydantic objects directly here? (Waypoint, SpaceTimeRegion, then building up to Schedule) I think it would be less error prone than manually creating the dictionary/YAML.

VeckoTheGecko · 2025-02-04T10:11:46Z

src/virtualship/utils.py

+    # Extract unique instruments from dataset
+    unique_instruments = np.unique(
+        np.hstack(coordinates_data["Instrument"].apply(lambda a: a.split(", ")).values)
+    )


I think this would just be clearer as a for loop.

unique_instruments = set() for row in ....: instruments = ...split(", ") unique_instruments |= set(instruments) # or just `unique_instruments = unique_instruments | set(instruments)`

That way there's no need to write the comment since its evident from the code

VeckoTheGecko · 2025-02-04T10:19:07Z

src/virtualship/cli/commands.py

+    if mfp_file:
+        # Generate schedule.yaml from the MPF file
+        click.echo(f"Generating schedule from {mfp_file}...")
+        mfp_to_yaml(
+            mfp_file, str(path)
+        )  # Pass the path to save in the correct directory


If mfp_to_yaml returned a string representing the yaml instead of doing the file modifications, then we can use schedule.write_text(...) like in the else clause. So something like

if mfp_file: # don't repeat the comment here :) the log message is comment enough click.echo(f"Generating schedule from {mfp_file}...") schedule_body = mfp_to_yaml( mfp_file, ) else: schedule_body = utils.get_example_schedule() schedule.write_text(...)

VeckoTheGecko · 2025-02-04T10:22:02Z

environment.yml

@@ -12,6 +12,7 @@ dependencies:
  - pip
  - pyyaml
  - copernicusmarine >= 2
+  - openpyxl >= 3.1.5


Suggested change

- openpyxl >= 3.1.5

- openpyxl

I don't think we need to specify the lower bound. Its best to be flexible unless there is an incompatibility with x version in our codebase

VeckoTheGecko · 2025-02-04T10:25:28Z

src/virtualship/utils.py

+    # Define maximum depth and buffer for each instrument
+    instrument_properties = {
+        "CTD": {"depth": 5000, "buffer": 1},
+        "DRIFTER": {"depth": 1, "buffer": 5},
+        "ARGO_FLOAT": {"depth": 2000, "buffer": 5},
+    }


Suggested change

# Define maximum depth and buffer for each instrument

instrument_properties = {

"CTD": {"depth": 5000, "buffer": 1},

"DRIFTER": {"depth": 1, "buffer": 5},

"ARGO_FLOAT": {"depth": 2000, "buffer": 5},

}

instrument_properties = {

"CTD": {"max_depth": 5000, "buffer": 1},

"DRIFTER": {"max_depth": 1, "buffer": 5},

"ARGO_FLOAT": {"max_depth": 2000, "buffer": 5},

}

VeckoTheGecko · 2025-02-04T10:26:16Z

src/virtualship/cli/commands.py

+            mfp_file, str(path)
+        )  # Pass the path to save in the correct directory
+    else:
+        # Create a default example schedule


Suggested change

# Create a default example schedule

iuryt and others added 2 commits January 28, 2025 13:40

draft script for converting MFP CSV to YAML schedule

5c5bf9a

[pre-commit.ci] auto fixes from pre-commit.com hooks

40c7ff9

for more information, see https://pre-commit.ci

iuryt changed the title ~~draft script for converting MFP CSV to YAML schedule~~ Converting MFP CSV to YAML schedule Jan 28, 2025

iuryt commented Jan 29, 2025

View reviewed changes

iuryt and others added 6 commits February 3, 2025 12:31

add openpyxl

366dede

add mfp_to_yaml function

d046444

add new command to init to accept mfp file as input

e3199fa

delete files from scripts/

d9fe46a

deleted scripts files

7dc9bd7

[pre-commit.ci] auto fixes from pre-commit.com hooks

a79433c

for more information, see https://pre-commit.ci

VeckoTheGecko requested changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting MFP CSV to YAML schedule #111

Converting MFP CSV to YAML schedule #111

iuryt commented Jan 28, 2025 •

edited

Loading

ammedd commented Jan 29, 2025

iuryt commented Jan 29, 2025

VeckoTheGecko commented Jan 29, 2025

iuryt Jan 29, 2025

VeckoTheGecko commented Jan 31, 2025 •

edited

Loading

iuryt commented Feb 3, 2025 •

edited

Loading

erikvansebille commented Feb 4, 2025

VeckoTheGecko left a comment •

edited

Loading

VeckoTheGecko Feb 4, 2025

VeckoTheGecko Feb 4, 2025

VeckoTheGecko Feb 4, 2025

VeckoTheGecko Feb 4, 2025

VeckoTheGecko Feb 4, 2025

VeckoTheGecko Feb 4, 2025

Converting MFP CSV to YAML schedule #111

Are you sure you want to change the base?

Converting MFP CSV to YAML schedule #111

Conversation

iuryt commented Jan 28, 2025 • edited Loading

ammedd commented Jan 29, 2025

iuryt commented Jan 29, 2025

VeckoTheGecko commented Jan 29, 2025

iuryt Jan 29, 2025

Choose a reason for hiding this comment

VeckoTheGecko commented Jan 31, 2025 • edited Loading

iuryt commented Feb 3, 2025 • edited Loading

erikvansebille commented Feb 4, 2025

VeckoTheGecko left a comment • edited Loading

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

VeckoTheGecko Feb 4, 2025

Choose a reason for hiding this comment

iuryt commented Jan 28, 2025 •

edited

Loading

VeckoTheGecko commented Jan 31, 2025 •

edited

Loading

iuryt commented Feb 3, 2025 •

edited

Loading

VeckoTheGecko left a comment •

edited

Loading