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

Get maxLevelCell from ds, not ds_mesh in viz #101

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Jul 27, 2023

This is because ds_mesh should only contain horizontal mesh variables.

Checklist

  • Testing comment in the PR documents testing used to verify the changes

@xylar xylar changed the title Fix max level cell in horiz viz Get maxLevelCell from ds, not ds_mesh in viz Jul 27, 2023
@xylar
Copy link
Collaborator Author

xylar commented Jul 27, 2023

@cbegeman, the ocean/baroclinic_channel/10km/default test case is currently failing with:

Exception raised while running the steps of the test case
Traceback (most recent call last):
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/polaris/run/serial.py", line 310, in _log_and_run_test
    _run_test(test_case, available_resources)
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/polaris/run/serial.py", line 409, in _run_test
    _run_step(test_case, step, test_case.new_step_log_file,
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/polaris/run/serial.py", line 484, in _run_step
    step.run()
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/polaris/ocean/tests/baroclinic_channel/viz.py", line 38, in run
    plot_horiz_field(ds, ds_mesh, 'temperature',
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/polaris/main/polaris/viz/__init__.py", line 87, in plot_horiz_field
    raise ValueError(
ValueError: maxLevelCell must be added to ds_mesh before plotting.

While one fix would be to add maxLevelCell to ds_mesh as has been done elsewhere, I think it makes more sense for ds_mesh to contain only horizontal mesh variables. I'm hoping you agree, but I also wasn't at recent OMEGA meetings where this may have been discussed.

In particular, I don't think maxLevelCell can be considered a mesh variable because it should be allowed to change in time (e.g. by remapping).

@xylar xylar requested a review from cbegeman July 27, 2023 09:45
This is because `ds_mesh` should only contain horizontal mesh
variables.
@xylar xylar force-pushed the fix-max-level-cell-in-horiz-viz branch from f131549 to 44c34c4 Compare July 27, 2023 10:22
@xylar
Copy link
Collaborator Author

xylar commented Jul 27, 2023

Testing

I ran this with the following tests and they all passed:

  ocean/baroclinic_channel/10km/default
  ocean/baroclinic_channel/10km/decomp
  ocean/baroclinic_channel/10km/restart
  ocean/baroclinic_channel/10km/threads
  ocean/inertial_gravity_wave/convergence
  ocean/manufactured_solution/convergence
  ocean/single_column/960km/cvmix
  ocean/single_column/960km/ideal_age

I had to manually check the viz steps for inertial_gravity_wave and manufactured_solution, since they don't run automatically in any test.

@xylar xylar self-assigned this Jul 27, 2023
@xylar xylar added bug Something isn't working ocean Related to ocean tests or analysis labels Jul 27, 2023
Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar I agree with your changes. I think this is a better approach.

@xylar
Copy link
Collaborator Author

xylar commented Jul 27, 2023

Great, thanks @cbegeman!

@xylar xylar merged commit 76c6700 into E3SM-Project:main Jul 27, 2023
5 checks passed
@xylar xylar deleted the fix-max-level-cell-in-horiz-viz branch July 27, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants