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.

Sign up for GitHub

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

Jump to bottom

[Refactor]: Stop default diagnostics always running if e3sm_diags is executed via Python script #735

Closed
3 tasks
tomvothecoder opened this issue Sep 29, 2023 · 0 comments · Fixed by #747
Closed
3 tasks

[Refactor]: Stop default diagnostics always running if e3sm_diags is executed via Python script #735

tomvothecoder opened this issue Sep 29, 2023 · 0 comments · Fixed by #747
Labels
possible enhancement refactor

Comments

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Sep 29, 2023 •
edited
Loading

Overview

If e3sm_diags is executed via a Python script, the default diagnostics will always execute and the CoreParameter.sets attribute defined in the Python script is ignored.

This means debugging can't be done on a single set/variable. Instead, this behavior should be fixed because debugging is much easier by executing a Python script that defines a single CoreParameter object with a single set and a few variables.

If the user wants to run the default sets, they should manually do that via the CLI (e.g., e3sm_diags -d lat_lon_model_vs_model.cfg).

Related Code

  • Remove the default value for CoreParameter.sets is a list of all of the sets:

    e3sm_diags/e3sm_diags/parameter/core_parameter.py

    Lines 55 to 76 in a2d00eb

    # A list of the sets to be run. Default is all sets
    self.sets: List[str] = [
    "zonal_mean_xy",
    "zonal_mean_2d",
    "zonal_mean_2d_stratosphere",
    "meridional_mean_2d",
    "lat_lon",
    "polar",
    "area_mean_time_series",
    "cosp_histogram",
    "enso_diags",
    "qbo",
    "streamflow",
    "diurnal_cycle",
    "arm_diags",
    "tc_analysis",
    "annual_cycle_zonal_mean",
    "lat_lon_land",
    "lat_lon_river",
    "aerosol_aeronet",
    "aerosol_budget",
    ]
  • Remove Run.sets_to_run = CoreParameter().sets default value.

    e3sm_diags/e3sm_diags/run.py

    Lines 20 to 22 in a2d00eb

    def __init__(self):
    self.sets_to_run = CoreParameter().sets
    self.parser = CoreParser()
  • Update Run.get_final_parameters() sets_to_run to get the sets from the defined Parameter objects via parameters arg

Original Discussion: #677 (comment)
@tomvothecoder tomvothecoder added this to the FY24 (Multi-Quarter) milestone Sep 29, 2023
@tomvothecoder tomvothecoder changed the title [Refactor] Consider refactoring CoreParameter.sets default value from all sets to None or empty list [Refactor] Consider refactoring e3sm_diags default behavior from running all sets to being explicitly specified Sep 29, 2023
@tomvothecoder tomvothecoder mentioned this issue Sep 29, 2023
Merged
9 tasks
@tomvothecoder tomvothecoder changed the title [Refactor] Consider refactoring e3sm_diags default behavior from running all sets to being explicitly specified [Refactor] Consider refactoring CoreParameter and Run classes to focus on a single set instead of list of sets Sep 29, 2023
@tomvothecoder tomvothecoder changed the title [Refactor] Consider refactoring CoreParameter and Run classes to focus on a single set instead of list of sets [Refactor] Consider refactoring CoreParameter and Run classes to focus on 1 parameter object per set Sep 29, 2023
@tomvothecoder tomvothecoder removed this from the FY24 (Multi-Quarter) milestone Oct 10, 2023
@tomvothecoder tomvothecoder changed the title [Refactor] Consider refactoring CoreParameter and Run classes to focus on 1 parameter object per set [Refactor]: Remove default diagnostic sets behavior when running e3sm_diags via Python scripts Oct 23, 2023
@tomvothecoder tomvothecoder changed the title [Refactor]: Remove default diagnostic sets behavior when running e3sm_diags via Python scripts [Refactor]: Remove running default diagnostic sets when running e3sm_diags via Python script Oct 23, 2023
@tomvothecoder tomvothecoder changed the title [Refactor]: Remove running default diagnostic sets when running e3sm_diags via Python script [Refactor]: Stop default diagnostic sets when running e3sm_diags via Python script Oct 23, 2023
@tomvothecoder tomvothecoder changed the title [Refactor]: Stop default diagnostic sets when running e3sm_diags via Python script [Refactor]: Stop default diagnostics always running if e3sm_diags is executed via Python script Oct 23, 2023
@tomvothecoder tomvothecoder mentioned this issue Oct 23, 2023
Merged
9 tasks
tomvothecoder added a commit that referenced this issue Dec 13, 2023
@tomvothecoder
[Refactor]: Refactor integration tests and add use_cfg flag to `run…
9e14ff8 
…_diags()` (#747)

* Refactor integration tests (_tests/integration_)
  * Delete `test_dataset.py` because it is a really old and incomplete test file for the legacy `Dataset` class based on CDAT
  * Delete `test_all_sets.py` and `all_sets_modified.cfg` because it tests for expected image counts which is redundant (`test_all_sets_image_diffs.py` does this already)
  * Replace `subprocess` calls with direct call to Python API for real-time test results and easier debugging
  * Move `complete_run.py` to `/test/integration`
  * Move `test_run.py` to `tests/e3sm_diags` since it is more of a unit test file
* Refactor `Run` class (_run.py_)
  * Closes #735 
  * Add `use_cfg` boolean argument to `run_diags()` and `get_run_parameters()`
  * Add `self.cfg_path` attribute, which is set if `.cfg` file(s) are used for diagnostic runs
  * Add `is_cfg_file_arg_set()` property to check parser for `-d/--diags` 
  * Rename `get_final_parameters()` to `get_run_parameters()` and refactored to smaller methods
* Update CI/CD build workflow (_build_workflow.yml_)
  * Split up testing step into: 1) run unit tests 2) download integration test data 3) run integration tests
  * Easier to keep track of runtime and results
chengzhuzhang pushed a commit that referenced this issue Feb 24, 2024
@tomvothecoder @chengzhuzhang
[Refactor]: Refactor integration tests and add use_cfg flag to `run…
ecc5f2a 
…_diags()` (#747)

* Refactor integration tests (_tests/integration_)
  * Delete `test_dataset.py` because it is a really old and incomplete test file for the legacy `Dataset` class based on CDAT
  * Delete `test_all_sets.py` and `all_sets_modified.cfg` because it tests for expected image counts which is redundant (`test_all_sets_image_diffs.py` does this already)
  * Replace `subprocess` calls with direct call to Python API for real-time test results and easier debugging
  * Move `complete_run.py` to `/test/integration`
  * Move `test_run.py` to `tests/e3sm_diags` since it is more of a unit test file
* Refactor `Run` class (_run.py_)
  * Closes #735 
  * Add `use_cfg` boolean argument to `run_diags()` and `get_run_parameters()`
  * Add `self.cfg_path` attribute, which is set if `.cfg` file(s) are used for diagnostic runs
  * Add `is_cfg_file_arg_set()` property to check parser for `-d/--diags` 
  * Rename `get_final_parameters()` to `get_run_parameters()` and refactored to smaller methods
* Update CI/CD build workflow (_build_workflow.yml_)
  * Split up testing step into: 1) run unit tests 2) download integration test data 3) run integration tests
  * Easier to keep track of runtime and results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
possible enhancement refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Refactor]: Refactor integration tests and add use_cfg flag to run_diags() E3SM-Project/e3sm_diags
1 participant
@tomvothecoder