Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config Cleanup #106

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Config Cleanup #106

merged 1 commit into from
Oct 29, 2024

Conversation

uwagura
Copy link
Collaborator

@uwagura uwagura commented Oct 28, 2024

This PR starts the process of making the config.yaml file a bit easier use now that it has settings for each of the physics diagnostic scripts. So far, I have just regrouped some of the entries so that similar settings are grouped together and the comments are clearer, but let me know if there are more changes we should make

@uwagura uwagura requested a review from yichengt900 October 28, 2024 20:35
@yichengt900
Copy link
Contributor

@uwagura, thanks for submitting this cleanup PR. I appreciate your initiative! I think we could take the refactoring a step further by grouping standalone keys that are used within the same script into cohesive dictionaries (or mappings). For instance, rather than accessing figures_dir directly, we could include it within a general section, which would then be accessed as general:figures_dir.

Below is a possible layout for the new config.yaml:

# General settings for dataset locations and figures
general:
    figures_dir: 'figures/'
    model_grid: '../data/geography/ocean_static.nc'

# Datasets paths
datasets:
    glorys: '/work/acr/mom6/diagnostics/glorys/glorys_sfc.nc'
    glorys_zos: '/work/acr/glorys/GLOBAL_MULTIYEAR_PHY_001_030/monthly/glorys_monthly_z_fine_*.nc'
    oisst: '/work/acr/oisstv2/'

# Variables renaming map
rename_map:
    geolon: lon
    geolat: lat
    geolon_c: lon_b
    geolat_c: lat_b

# Domain and general plotting details
plotting:
    domain: ocean_monthly
    lat_range:
        south: 0
        north: 60
    lon_range:
        west: 260
        east: 330
    xlim:
        min: -99
        max: -35
    ylim:
        min: 4
        max: 59
    projection:
        grid: 'PlateCarree'
        data: 'PlateCarree'
    figure_size:
        width: 11
        height: 14
    skill_score_text:
        x: -98.5
        y: 54
        xint: 4
        yint: 4
    plot_lat: False

# SST Evaluation and Trend settings
sst_eval:
    colorbar:
        levels_min: 2
        levels_max: 31
        levels_step: 2
    colorbar_diff:
        bias_min: -2
        bias_max: 2.1
        bias_step: 0.25
        ticks: [-2, -1, 0, 1, 2]

# SST Trends settings
sst_trends:
    start_year: "2005"
    end_year: "2019"
    colorbar_diff:
        bias_min: -2
        bias_max: 2.1
        bias_step: 0.25
        bias_min_trends: -1.5
        bias_max_trends: 1.51
        ticks: [-2, -1, 0, 1, 2]

# SSS Evaluation settings
sss_eval:
    rcdir: '/net2/acr/regional_climatologies/'
    rc_vars:
        - swa
        - gom
        - nwa

# SSH Evaluation settings
ssh_eval:
    colorbar:
        levels_min: -1.1
        levels_max: 0.8
        levels_step: 0.1

This structure could help by consolidating settings relevant to each script, improving readability and maintainability. These are just some initial thoughts—I might have overlooked something. Also, this will require some changes in the Python source code to adapt to the new structure. We can discuss further tomorrow. Thanks again for getting the ball rolling on this!

@yichengt900
Copy link
Contributor

@uwagura, as discussed this morning, I'll go ahead and approve this PR for now and hold off on further config.yaml refactoring until we complete the primary code cleanup. I still think that consolidating the configuration file using a dictionary structure would enhance clarity, but it’s not a priority at this stage. Let’s plan to revisit this improvement later.

Copy link
Contributor

@yichengt900 yichengt900 left a comment

Choose a reason for hiding this comment

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

Changes look good. I’ve added a comment regarding potential future plans. Approving this PR for now

@yichengt900 yichengt900 merged commit 9aa885d into main Oct 29, 2024
2 checks passed
@yichengt900 yichengt900 deleted the cleanup_config branch October 31, 2024 16:57
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