Skip to content

Conversation

@pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Oct 14, 2023


name: "Make tests basis-independent"
description: ''
title: ''
labels: ''
assignees: ''


Description

We need to drop the first commit and re-run CI with the equispaced variant.

A few observations:

  1. VertexLimiter requires functions in Broken(mesh.coordinates.function_space())
  2. Some tests like the one for EquationBC use a periodic sine wave manufactured solution, this could show the asymptotic convergence rate sooner with equispaced points (these points have the best approximation properties for periodic functions).
  3. The tests for the adjoint of cross-mesh interpolation were testing for inequality of the output against the exact target Cofunction, and the source Function. The issue with this is that the absolute tolerance used in the dual comparison was the same used for primal comparsion, and the change to the spectral basis was changing the magnitude of the coefficients of the Cofunctions below the absolute tolerance. These inequality tests are quite meaningless and should be replaced with the same approach used in the multigrid restrict tests. We now test that <Ix, y> == <x, adjoint(I)y> for a choice of x \in V_src and y \in V_dest*.

Associated Pull Requests:

Fixes Issues:

If issues are fixed by this PR, prepend each of them with the word
"fixes", so they are automatically closed when this PR is merged. For
example "fixes #123, fixes #456".

Checklist for author:

  • I have linted the codebase (make lint in the firedrake source directory).
  • My changes generate no new warnings.
  • All of my functions and classes have appropriate docstrings.
  • I have commented my code where its purpose may be unclear.
  • I have included and updated any relevant documentation.
  • Documentation builds locally (make linkcheck; make html; make latexpdf in firedrake/docs directory)
  • I have added tests specific to the issues fixed in this PR.
  • I have added tests that exercise the new functionality I have introduced
  • Tests pass locally (pytest tests in the firedrake source directory) (useful, but not essential if you don't have suitable hardware).
  • I have performed a self-review of my own code using the below guidelines.

Checklist for reviewer:

  • Docstrings present
  • New tests present
  • Code correctly commented
  • No bad "code smells"
  • No issues in parallel
  • No CI issues (excessive parallelism/memory usage/time/warnings generated)
  • Upstream/dependent branches and PRs are ready

Feel free to add reviewers if you know there is someone who is already aware of this work.

Please open this PR initially as a draft and mark as ready for review once CI tests are passing.

@pbrubeck pbrubeck force-pushed the pbrubeck/test-simplex-spectral branch from d98024f to a6db605 Compare October 17, 2023 17:42
@pbrubeck pbrubeck changed the title [DO NOT MERGE] Test spectral as default variant on simplices Test spectral as default variant on simplices Oct 18, 2023
@pbrubeck pbrubeck changed the title Test spectral as default variant on simplices Make tests basis-independent Oct 25, 2023
Copy link
Member

@dham dham left a comment

Choose a reason for hiding this comment

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

Just one trivial change.

@pbrubeck pbrubeck force-pushed the pbrubeck/test-simplex-spectral branch 4 times, most recently from f45ad60 to d746a77 Compare October 25, 2023 16:06
@pbrubeck pbrubeck force-pushed the pbrubeck/test-simplex-spectral branch from d746a77 to af6f340 Compare October 25, 2023 16:09
@pbrubeck pbrubeck requested a review from dham October 25, 2023 21:37
Copy link
Contributor

@ReubenHill ReubenHill left a comment

Choose a reason for hiding this comment

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

This looks good to go now

@ReubenHill ReubenHill enabled auto-merge (squash) October 27, 2023 10:57
@ReubenHill ReubenHill disabled auto-merge October 27, 2023 10:57
@ReubenHill ReubenHill merged commit ff43fc8 into master Oct 27, 2023
@ReubenHill ReubenHill deleted the pbrubeck/test-simplex-spectral branch October 27, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for local_range

5 participants