Add and test function-based indefinite integrals #1504
Conversation
charleskawczynski
requested review from
simonbyrne,
dennisYatunin and
Sbozzolo
I didn't add a convergence test, that'd probably be a good idea to have, generally, though. I can confirm that the new operator is 2nd order accurate (by adjusting the existing test and commenting out some printed things), but adding that test does make the test suite a bit more complicated (we're already looping over float type, functions to integrate, I unrolled the loop over spaces to make debugging easier, adding a loop over number of vertical elements will put it back to cubic). I'm fine with adding it if you think it's worth it. |
charleskawczynski
force-pushed
the
ck/column_ops2
branch
from
67e6e02
to
f6651dd
Compare
For simple operations like this, I think that it would be a stronger test of correctness. We probably just need to run the same test with twice as many vertical elements and check the correct scaling in the error, and this might be a second for loop along with |
I agree. In the interest in reducing package load times sooner than later, I'll open an issue to add convergence tests. Maybe they can be added more simply in some way. |
|
@Sbozzolo, alright to merge? |
|
Opened #1508. |
Can you please add line to explain in the docstring what |
charleskawczynski
force-pushed
the
ck/column_ops2
branch
from
f6651dd
to
f144a9a
Compare
Ah, thanks. I did update it but I must have accidentally stashed it. Updated, can you take another look @Sbozzolo? |
|
bors r+ |
1504: Add and test function-based indefinite integrals r=charleskawczynski a=charleskawczynski This PR adds an indefinite integral implementation that uses a function-based input argument. Concretely, given `ϕ(z)`, this interface allows us to solve for `∂ϕ/∂z = f(ϕ,z)`. The existing field-based implementation cannot be used in this situation, because `f` is a function of `ϕ`, which is what we're computing. So, this implementation uses RootSolvers to find `ϕ` at the next level in order to provide a somewhat generic interface. Unfortunately, one limitation of this is that `ϕ` must be a scalar field, and it cannot be, e.g., a `NamedTuple`. We could try to lift this assumption, but that would require adjustments to RootSolvers. Closes #1492. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
|
Build failed: |
|
bors r+ |
1504: Add and test function-based indefinite integrals r=charleskawczynski a=charleskawczynski This PR adds an indefinite integral implementation that uses a function-based input argument. Concretely, given `ϕ(z)`, this interface allows us to solve for `∂ϕ/∂z = f(ϕ,z)`. The existing field-based implementation cannot be used in this situation, because `f` is a function of `ϕ`, which is what we're computing. So, this implementation uses RootSolvers to find `ϕ` at the next level in order to provide a somewhat generic interface. Unfortunately, one limitation of this is that `ϕ` must be a scalar field, and it cannot be, e.g., a `NamedTuple`. We could try to lift this assumption, but that would require adjustments to RootSolvers. Closes #1492. 1515: remove distributed TempestRemap tests r=charleskawczynski a=simonbyrne This isn't how we should do distributed remapping, so i'll just disable them. Avoids #1513. - [x] Code follows the [style guidelines](https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/) OR N/A. - [x] Unit tests are included OR N/A. - [x] Code is exercised in an integration test OR N/A. - [x] Documentation has been added/updated OR N/A. Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com> Co-authored-by: Simon Byrne <simonbyrne@gmail.com>
|
Build failed (retrying...): |
charleskawczynski
force-pushed
the
ck/column_ops2
branch
from
f144a9a
to
67af7a9
Compare
|
Canceled. |
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
This PR adds an indefinite integral implementation that uses a function-based input argument. Concretely, given
ϕ(z), this interface allows us to solve for∂ϕ/∂z = f(ϕ,z). The existing field-based implementation cannot be used in this situation, becausefis a function ofϕ, which is what we're computing. So, this implementation uses RootSolvers to findϕat the next level in order to provide a somewhat generic interface.Unfortunately, one limitation of this is that
ϕmust be a scalar field, and it cannot be, e.g., aNamedTuple. We could try to lift this assumption, but that would require adjustments to RootSolvers.Closes #1492.