Develop#14
Merged
Merged
Conversation
Added detailed information on datasets used for calibration workflow, including in-situ and satellite altimetry data. Included tables and access links for both dataset types.
Move load_configuration.py and deletes src/config folder due to overlapping with top-level config folder
Updated directory paths in the README to remove relative references.
Updated the README to include detailed descriptions of in-situ and satellite altimetry data, including access instructions and dataset information.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves project automation and packaging, standardizes configured data paths, expands dataset documentation, and replaces ad-hoc prints with structured logging across the extraction/matching/export workflow.
Changes:
- Added Python CI workflow and introduced
pyproject.tomlfor packaging/installation. - Standardized data path references in
config/config.yamlandREADME.md, and added extensive dataset documentation underdata/. - Refactored processing/export code to use a shared logger and adjusted calibration/bias-correction behavior.
Reviewed changes
Copilot reviewed 16 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/python-app.yml |
Adds GitHub Actions CI to install, lint, and run tests (if present). |
.gitignore |
Ignores build artifacts and logs/. |
README.md |
Updates docs (paths, installation section typo fix, workflow note). |
config/config.yaml |
Standardizes relative paths (removes leading ../). |
data/README.md |
Adds detailed dataset selection/access documentation. |
logs/thesis.log |
Adds a generated log file (should not be committed). |
pyproject.toml |
Adds packaging metadata, dependencies, and a console script entry point. |
src/utils/logger_setup.py |
Introduces a shared logger writing to logs/thesis.log. |
src/load_configuration.py |
Switches config loading to logging-based behavior (currently breaks API contract). |
src/main.py |
Updates config path usage and imports (currently broken due to renamed calibration API). |
src/run_extraction.py |
Updates config import (still not install-safe with new packaging). |
src/io_data/load_dataframe.py |
Adds logging + warning suppression; changes error handling (currently returns None on failure). |
src/io_data/export_data_to_csv.py |
Replaces prints with logger calls; adds basic export logging. |
src/processing/processing_df.py |
Removes tqdm usage for iteration. |
src/processing/spatial_matching.py |
Adds logging and empty-DataFrame checks (currently has incorrect return/logging behavior). |
src/processing/temporal_matching.py |
Adds logging and empty-DataFrame checks (currently contains runtime errors and missing returns). |
src/calibration/calibration_methods.py |
Renames first-ten-days calibration to first-twenty-days and updates masks (doc mismatch, call sites not updated). |
src/calibration/bc_techniques.py |
Fixes FDM assignment and increases QM polyfit degree (now risks crashing on small bins). |
Comments suppressed due to low confidence (6)
src/calibration/calibration_methods.py:8
- The function was renamed to
calib_df_first_twenty_days, but the docstring and inline comment still describe using the “first ten days”. This is misleading documentation and makes it easy for callers to use the wrong split logic; update the docstring/comment to match the 20-day behavior (or rename back if 10 days is intended).
def calib_df_first_twenty_days(df_mooring: pd.DataFrame, df_sat: pd.DataFrame) \
-> tuple[pd.DataFrame, pd.DataFrame, pd.DataFrame, pd.DataFrame]:
"""
Calibrates the satellite data using the first ten days of mooring data as the calibration set and the remaining
days as the validation set. The function creates boolean masks based on the day of the month extracted from the
src/load_configuration.py:21
- If the config path does not exist, this logs a warning but continues into
path.open(...), which will raiseFileNotFoundError(not caught here). Either raise immediately (as before) or return a safe default to avoid an unexpected exception path.
src/main.py:3 - This module uses top-level imports like
import load_configuration/from processing import ..., which only work ifsrc/is onPYTHONPATH(e.g., running from withinsrc). With the newpyproject.tomlpackaging, the recommended approach is to import from the installed package namespace (e.g.,<package>.load_configuration,<package>.processing) sothesis-runworks afterpip install ..
src/main.py:14 calib_df_first_ten_daysis still referenced later in this file, butcalibration_methods.pynow definescalib_df_first_twenty_daysinstead. This will breakmain()at runtime with anAttributeError; update the call site(s) or keep a compatibility alias incalibration_methods.py.
src/load_configuration.py:25- On
yaml.YAMLError, the function logs but does not raise or return a dict, so it implicitly returnsNonedespite the-> dictannotation. This can cause downstreamTypeError/KeyErrorfailures; raise aValueError(or return{}) after logging to keep the API contract consistent.
src/run_extraction.py:4 import load_configurationrelies onsrc/being onPYTHONPATHand won’t be importable when the project is installed as a package viapyproject.toml(unlessload_configuration.pyis included as an installed module). Consider moving this into a real package namespace and importing via that namespace so it works both in CI and for end users.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements to project structure, configuration, documentation, and code reliability. The main highlights include the addition of a Python CI workflow, standardization of data paths across configuration and documentation, enhanced logging for data export operations, and expanded documentation on dataset usage and sources.
Project Infrastructure and Automation:
.github/workflows/python-app.yml)pyproject.tomlfor standardized Python project configuration and dependency management, specifying required packages and entry points.Configuration and Documentation Consistency:
config/config.yaml) and documentation (README.md) by removing leading../, ensuring consistency and reducing path errors. [1] [2] [3] [4] [5]../for clarity and correctness.Expanded Dataset Documentation:
data/README.mddetailing the selection, characteristics, and access instructions for both in-situ and satellite datasets, including platform lists, images, and links to data sources.Code Quality and Reliability Enhancements:
src/io_data/export_data_to_csv.py) [1] [2] [3]src/io_data/load_dataframe.py)Calibration Method Adjustments:
src/calibration/calibration_methods.py) [1] [2] [3]src/calibration/bc_techniques.py)src/calibration/bc_techniques.py)These changes collectively improve the project's maintainability, reliability, and clarity for both users and contributors.