Skip to content

Comments

Scatter review#1

Open
stkistner wants to merge 18 commits intoRasmusiwersen:mainfrom
stkistner:scatter_review
Open

Scatter review#1
stkistner wants to merge 18 commits intoRasmusiwersen:mainfrom
stkistner:scatter_review

Conversation

@stkistner
Copy link

@stkistner stkistner commented Dec 1, 2025

First, auto-review with copilot

✅ Added logging support - Logger configured for better error tracking
✅ Fixed month range bug - Uses pd.date_range() instead of np.arange() to handle multi-year periods correctly
✅ Replaced bare excepts - All bare except: clauses now catch specific exceptions with proper error messages
✅ Cross-platform paths - Uses pathlib.Path instead of hardcoded Windows path separators
✅ Validated bbox coordinates - Added validation before float conversion to prevent crashes
✅ Fixed reset_index - Now uses drop=True to avoid creating unwanted index column
✅ Better error messages - Changed print statements to logger calls with appropriate levels
✅ Fixed output_directory - Converts Path objects to strings for API calls


Then., some changes:

  • download_copernicus_data -> get_satobs_data (more aligned with get_altimetry_data)
  • More unification with exisiting altimery class
  • Single & multipledataset downloads, etc.

@stkistner stkistner marked this pull request as ready for review December 2, 2025 13:59
"### Specify dataset of product to be downloaded from CMEMS Online Catalogue\n",
"- Click through the catalogue here: https://data.marine.copernicus.eu/products?pk_vid=bfd50509683fc0421764148768aca348 \n",
"\n",
"Options are currently:\n",
Copy link
Owner

Choose a reason for hiding this comment

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

The download functionality is not limited to these products. They are just the only ones we are certain to be able to convert to dataframe with the current repo. You could specify what ever, and have it download and then manually convert the downloaded data to whatever format you would need.

df = df.rename(columns={"VAVH": "SWH", "VAVH_UNFILTERED": "SWH_UNFILTERED"})
return df

def _cmems_format_raw_data(self, temp_dir, file_path):
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps print('Converting raw CMEMS data to dataframe') or something similar. Right now the code displays "Download successful" even though the code keeps running which i guess can be slightly confusing.

@stkistner
Copy link
Author

Let me make some more changes...

@stkistner
Copy link
Author

stkistner commented Dec 3, 2025

Changes made (and still adding to this list):

single vs multi dataset Download:

This new implementation simplifies things down the road, like downloading meta data before actually downloading data. This makes it easier to QC the start/end date before downloading....

repo = CMEMSSatObsRepository(dataset_id='cmems_obs-wind_glo_phy_nrt_l3-hy2b-hscat-asc-0.25deg_P1D-i') #single
# or
repo = CMEMSSatObsRepository(product_id="WIND_GLO_PHY_L3_NRT_012_002") #multi
...

df = repo.get_satobs_data() # Don't specify any dataset_id here...

Added checks for start/end dates and comparison to meta-data

Makes sure that the dates are aligned with the data, before starting a fallback to full dataset

Addedtmpdir

Changed tempfile.mkdtemp to a tempfile.TemporaryDirectory, wrapped inside an try-except block. This makes sure the directory gets deleted, even if the download script fails.

Added more shared functions to _DHISatMixin

As per title, more duplicate functions were found and moved to Mixin

AltimetryData object

As per DHIAltimetryRepository, returns an AltimetryData object. We may need to rename this class...

Updated Notebook

Notebook updated accordingly

@stkistner
Copy link
Author

#Todo:

  • Add quality flags for wind data too
  • Make sure item names are aligned with AltimetryData expected columns. Again, we may need to modify this class.

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.

2 participants