Skip to content

Fix eighteen broken coastal test cases #662

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
Merged
mark-petersen merged 0 commits into from
Aug 27, 2020
Merged

Fix eighteen broken coastal test cases #662

mark-petersen merged 0 commits into from
Aug 27, 2020

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Aug 23, 2020

This merge removes the e3sm_coupling step from the coastal/Maine test case. It is not configured properly and is not needed.

It also fixes links to cull_mesh.py in two USDEQU120cr10rr2 tests.

@xylar xylar requested review from mark-petersen and sbrus89 August 23, 2020 13:23
@xylar xylar mentioned this pull request Aug 23, 2020
Merged
mark-petersen
mark-petersen approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

I don't have time to test, but if it works for you @xylar go ahead and merge with the green button when you are ready.

@mark-petersen
Copy link
Contributor

mark-petersen commented Aug 24, 2020

@sbrus89 could you approve? Visual inspection is probably fine here.

@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020

I don't have time to test, but if it works for you @xylar go ahead and merge

Visual inspection is probably fine here.

I'm concerned that this attitude is becoming pervasive in COMPASS. As a result, tests are frequently broken like here and it makes them impossible to use as a baseline. As it is, I have to test several dozen test cases in #563 to make sure that work has not done any harm. Each broken test case adds considerable additional work to that process, including PRs like this one to fix them.

As I have complained in the past, it becomes very difficult to maintain the COMPASS infrastructure if the test cases are not exercised on a regular basis and that job cannot fall exclusively to me.

@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020

I don't have time to test

This one irks me especially. I do not have time to fix these tests (they are not mine!) but I took the time anyway and I will take the time to test them each twice more than I would have had to if they worked.

@xylar xylar changed the title Fix three coastal test cases Fix eight coastal test cases Aug 24, 2020
@xylar xylar force-pushed the ocean/fix_broken_compass_tests branch from efe9d26 to 4a1cfb1 Compare August 24, 2020 10:55
@xylar xylar changed the title Fix eight coastal test cases Fix fourteen broken coastal test cases Aug 24, 2020
@xylar xylar changed the title Fix fourteen broken coastal test cases Fix a large number broken coastal test cases Aug 24, 2020
@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020 •
edited
Loading

Testing

Tests that now run successfully:

  • Gaussian_hump/USDEQU120cr10rr2/build_mesh/
  • coastal/Maine/init
  • coastal/USDEQU120cr10rr2/build_mesh
  • global_ocean/CA120to3/build_mesh
  • global_ocean/HI120to12/build_mesh
  • hurricane/USDEQU120at30cr10rr2/build_mesh
  • hurricane/USDEQU120at30cr10rr2WD/build_mesh
  • hurricane/USDEQU120at30cr10rr2WD_veg/build_mesh
  • hurricane/USDEQU240at60cr20rr4/build_mesh
  • hurricane/USDEQU240at60cr20rr4WD/build_mesh
  • hurricane/USDEQU240at60cr20rr4WD_veg/build_mesh
  • hurricane/USDEQU60at15cr5rr1/build_mesh
  • hurricane/USDEQU60at15cr5rr100WD/build_mesh
  • hurricane/USDEQU60at15cr5rr1WD/build_mesh
  • hurricane/USDEQU60at15cr5rr1WD_veg/build_mesh
  • hurricane/USDEQU60at15cr5rr250WD/build_mesh
  • hurricane/USDEQU60at15cr5rr500WD/build_mesh
  • tides/USDEQU120at30cr10/build_mesh

@xylar xylar changed the title Fix a large number broken coastal test cases Fix eighteen broken coastal test cases Aug 24, 2020
@xylar xylar force-pushed the ocean/fix_broken_compass_tests branch from 4a1cfb1 to 66408f3 Compare August 24, 2020 13:26
@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020

@sbrus89, while there are 2 tests of mine still running, I think this is ready for you to review. My preference would be for you to set up and re-run these tests yourself and make sure you also see them working. If that's not possible, let's chat about how to review these fixes meaningfully with more than just a glance at the code changes.

@sbrus89
Copy link

sbrus89 commented Aug 24, 2020

@xylar, thanks very much for your help maintaining these. I will take a look. Can we possibly do this PR into ocean/coastal? Some of these issues may have already been cleaned up there.

@sbrus89
Copy link

sbrus89 commented Aug 24, 2020 •
edited
Loading

@xylar, nevermind... obviously, #485 has already been merged. Sorry about that.

@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020

Some of these issues may have already been cleaned up there.

I think these issues should have been dealt with in #485. I am not happy that I didn't get a chance to do this level of testing before approving that PR and I will not make that mistake again. Since we are considering ocean/coastal to be "closed" for the moment, I'd like to make the PR into ocean/develop.

@xylar
Copy link
Collaborator Author

xylar commented Aug 24, 2020

FYI, my testing is in:

/lustre/scratch4/turquoise/xylar/test_fix_broken/

@xylar xylar removed the Don't merge label Aug 25, 2020
@xylar
Copy link
Collaborator Author

xylar commented Aug 25, 2020

The hurricane/USDEQU60at15cr5rr100WD/build_mesh/culled_mesh step is taking more than 6 hours. We really need to improve our tools!

@mark-petersen
Copy link
Contributor

mark-petersen commented Aug 26, 2020

See #563. Tested with that one, passed nightly regression suite with compass 0.1.9. Also passed QU240wISC and USDEQU240at60cr20rr4/build_mesh on grizzly.

@sbrus89 I'll merge with your approval.

@xylar
Copy link
Collaborator Author

xylar commented Aug 26, 2020

This one is fine to merge before compass_0.1.9 is created.

sbrus89
sbrus89 approved these changes Aug 26, 2020
View reviewed changes
Copy link

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

Approved after testing:

-o ocean -c hurricane -r USDEQU240at60cr20rr4WD -t build_mesh
-o ocean -c hurricane -r USDEQU120at30cr10rr2WD -t build_mesh
-o ocean -c tides -r USDEQU120at30cr10 -t build_mesh

Tested with branch that merges #563 and #662 on Grizzly using test_compass_0.1.9

@mark-petersen mark-petersen merged commit b33b347 into MPAS-Dev:ocean/develop Aug 27, 2020
@xylar xylar deleted the ocean/fix_broken_compass_tests branch August 27, 2020 17:30
mark-petersen added a commit that referenced this pull request Aug 27, 2020
@mark-petersen
Merge 'ocean/develop' into e3sm/develop
2dab9c3 
Merge PR #563 'xylar/ocean/remove_jigsaw_to_mpas' into ocean/develop
Merge PR #662 'xylar/ocean/fix_broken_compass_tests' into ocean/develop
Merge PR #664 'xylar/ocean/fix_plot_initial_state' into ocean/develop
@mark-petersen mark-petersen mentioned this pull request Aug 27, 2020
Merged
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Aug 27, 2020
@jonbob
Merge branch 'mark-petersen/mpas/update_compass_200827' into next (PR #…
aedb303 
…3800)

Update mpas-source: compass only

This PR brings in a new mpas-source submodule with changes only to the
ocean core. All of the changes are to the COMPASS testing code that is
not used in E3SM, so this PR should have no effect on E3SM. The COMPASS
commits are:
* MPAS-Dev/MPAS-Model#563 'xylar/ocean/remove_jigsaw_to_mpas'
* MPAS-Dev/MPAS-Model#662 'xylar/ocean/fix_broken_compass_tests'
* MPAS-Dev/MPAS-Model#664 'xylar/ocean/fix_plot_initial_state'

[BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Aug 28, 2020
@jonbob
Merge branch 'mark-petersen/mpas/update_compass_200827' (PR #3800)
8a28b9d 
Update mpas-source: compass only

This PR brings in a new mpas-source submodule with changes only to the
ocean core. All of the changes are to the COMPASS testing code that is
not used in E3SM, so this PR should have no effect on E3SM. The COMPASS
commits are:
* MPAS-Dev/MPAS-Model#563 'xylar/ocean/remove_jigsaw_to_mpas'
* MPAS-Dev/MPAS-Model#662 'xylar/ocean/fix_broken_compass_tests'
* MPAS-Dev/MPAS-Model#664 'xylar/ocean/fix_plot_initial_state'

[BFB]
caozd999 pushed a commit to caozd999/MPAS-Model that referenced this pull request Jan 14, 2021
@mark-petersen
Merge PR MPAS-Dev#662 'xylar/ocean/fix_broken_compass_tests' into oce…
51c6177 
…an/develop

Fix eighteen broken coastal test cases MPAS-Dev#662

This merge removes the e3sm_coupling step from the coastal/Maine test
case. It is not configured properly and is not needed.

It also fixes links to cull_mesh.py in two USDEQU120cr10rr2 tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mark-petersen mark-petersen mark-petersen approved these changes

+1 more reviewer

@sbrus89 sbrus89 sbrus89 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mark-petersen mark-petersen

Labels

bug COMPASS Ocean

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xylar @mark-petersen @sbrus89 @matthewhoffman