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

cosine bell in pr test suite "takes too long" #241

Closed
xylar opened this issue Sep 23, 2021 · 9 comments
Closed

cosine bell in pr test suite "takes too long" #241

xylar opened this issue Sep 23, 2021 · 9 comments
Assignees
Labels
clean-up ocean python package DEPRECATED: PRs and Issues involving the python package (master branch)

Comments

@xylar
Copy link
Collaborator

xylar commented Sep 23, 2021

@mark-petersen mentioned in E3SM-Project/E3SM#4552 (comment)

Passes pr suite, matches compass baselines for optimized versions (except didn't test cosine bell because it takes too long).

We should make the necessary changes to the cosine bell test doesn't have to be skipped.

@xylar
Copy link
Collaborator Author

xylar commented Sep 23, 2021

On Chrysalis on 2 nodes, the cosine bell test takes 3:48 out of 16:22 total for the pr test suite, which seemed acceptable to me. However, if we need to reduce the highest resolution(s) from 60 km to 90 km or 120 km, that might be okay.

@mark-petersen, can you clarify what would need to change for this test to be usable for you?

@vanroekel, would reducing the highest resolution be acceptable or would it negate the usefulness of the test?

Obviously, we will improve performance in the long run using parsl but that project will need at least several more months.

@xylar xylar self-assigned this Sep 23, 2021
@xylar xylar added clean-up ocean python package DEPRECATED: PRs and Issues involving the python package (master branch) labels Sep 23, 2021
@mark-petersen
Copy link
Collaborator

@xylar thanks for posting this. My preference is the 150km. Then we still have four resolutions, which is fine for a convergence test:

QU150  QU180  QU210  QU240

The QU120 forward step takes 2:43 debug and 1:00 minutes with gnu optimized on grizzly. That doesn't seem worth it for a fifth point.

@xylar
Copy link
Collaborator Author

xylar commented Sep 23, 2021

Then we still have four resolutions, which is fine for a convergence test

The problem is that the error at coarse resolution saturates near 1.0 so this statement isn't really accurate. You might determine that the order of convergence is lower than it really is if you use resolutions that are too coarse. As a result, you might be more tolerant for errors that reduce the order of convergence in the future. So I hesitate to rely on such coarse resolutions. I think 3 minutes is still an acceptable amount of time for the test to take.

@xylar
Copy link
Collaborator Author

xylar commented Sep 23, 2021

The QU120 forward step takes 2:43 debug and 1:00 minutes with gnu optimized on grizzly. That doesn't seem worth it for a fifth point.

I don't think we want to run the pr test suite in debug mode. I would only run the nightly suite in debug mode. The point of the pr suite is for tests that are more costly (therefore maybe not for debug mode) but more rigorous than we were previously doing.

@vanroekel, could you weigh in? @ambrad, do you have comments?

@vanroekel
Copy link
Collaborator

Sorry to be a bit pedantic/negative, but I personally don't think 3:48 is a huge burden for a test. My view of this suite is it only gets run when a PR is made and only needs to be run once. 16min of testing doesn't seem onerous to me especially given its only 2 nodes, which seems it won't get in the way of other simulations/testing.

I also agree with @xylar that we don't want to run the pr suite in debug. It seems more appropriate for the things nightly catches.

Finally, looking at the plot here -- #111 -- I'm not convinced either we can get away with fewer resolutions, especially just 4.

@xylar
Copy link
Collaborator Author

xylar commented Sep 23, 2021

This plot shows what I'm talking about with regard to error saturating at coarse resolution:
#111 (comment)

And I fully agree with @vanroekel having just looked at the plot at #111 (comment) that I don't think the 4 coarsest resolutions can be trusted to give an accurate estimate of the rate of convergence.

@xylar
Copy link
Collaborator Author

xylar commented Sep 23, 2021

@mark-petersen, I realize Grizzly is slower than Chrysalis. What kind of timing do you see in optimized for the full test case? If it's too expensive, maybe it's time to move your testing to Chrysalis or use more nodes or something?

@mark-petersen
Copy link
Collaborator

OK, looks like I should change my testing method to use a light-weight scheme up until the final one, and try to use pr suite as a final check, and never with debug. I can do that. The inconvenience is that, when reviewing PRs, one often thinks a test is final, but in fact requires several code iterations. I can try testing on chrysalis as well.

@xylar
Copy link
Collaborator Author

xylar commented Sep 24, 2021

The inconvenience is that, when reviewing PRs, one often thinks a test is final, but in fact requires several code iterations.

I'd think there's no harm in using nightly first and running pr only when nightly succeeds. That would save you 10 minutes on each run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up ocean python package DEPRECATED: PRs and Issues involving the python package (master branch)
Projects
None yet
Development

No branches or pull requests

3 participants