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

Remove manufactured_soluiton test case from suites #92

Merged

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Jun 23, 2023

I think we want to remove this test from our suites for now since it is failing "on purpose" (i.e. we know the MPAS-Ocean code needs to be fixed to allows the solution to converge at the desired rate) but this is confusing for those who are trying to use these suites to validate unrelated code.

Checklist

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

closes #89

I think we want to remove this test from our suites for now since
it is failing "on purpose" (i.e. we know the MPAS-Ocean code needs
to be fixed to allows the solution to converge at the desired
rate) but this is confusing for those who are trying to use these
suites to validate unrelated code.
@xylar xylar added clean-up ocean Related to ocean tests or analysis labels Jun 23, 2023
@xylar xylar self-assigned this Jun 23, 2023
Copy link
Contributor

@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.

I agree. I'm finishing up verifying the fix in E3SM-Ocean-Discussion/E3SM#55 for time varying forcing and then that should be set to move over to a E3SM-Project PR. Hopefully we can get this resolved in the next week or two.

@xylar
Copy link
Collaborator Author

xylar commented Jun 23, 2023

@cbegeman, just wanted to make sure this is okay with you, too. I haven't tested this yet but will as soon as I can.

@xylar
Copy link
Collaborator Author

xylar commented Jun 23, 2023

@altheaden, if you want, you can test if this fixes the problem you were having, but I'll test it as well.

@altheaden
Copy link
Collaborator

@xylar I did some testing last night in a branch with the exact change you've made here, and it worked perfectly.

@xylar xylar removed the request for review from cbegeman June 23, 2023 19:13
@xylar
Copy link
Collaborator Author

xylar commented Jun 23, 2023

Testing

I tested this with the pr suite and the test didn't run as expected. I think this and @altheaden's testing is enough.

@xylar
Copy link
Collaborator Author

xylar commented Jun 23, 2023

@cbegeman, I know you're away having fun so don't worry about this one. We can turn this testing back on once you're back.

@xylar xylar merged commit d4f4a33 into E3SM-Project:main Jun 23, 2023
@xylar xylar deleted the remove-manufactured-solutions-from-pr branch June 23, 2023 19:17
@xylar
Copy link
Collaborator Author

xylar commented Jun 23, 2023

Thanks @sbrus89 and @altheaden!

@cbegeman
Copy link
Collaborator

@xylar and @altheaden Thanks for taking care of this while I was away. I don't have any problem with waiting to add this back to the suites until after we get convergence with MPAS-O.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Known issue in ocean_manufactured_solution_convergence causing PR suite to fail
4 participants