Allow Fourier and periodic rational derivative operators for BBMBBMEquations1D and SvaerdKalischEquations1D
#154
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
=======================================
Coverage 97.97% 97.97%
=======================================
Files 19 19
Lines 1776 1776
=======================================
Hits 1740 1740
Misses 36 36 ☔ View full report in Codecov by Sentry. |
| # test PeriodicRationalDerivativeOperator | ||
| D1 = periodic_derivative_operator(1, accuracy_order, xmin(mesh), xmax(mesh), | ||
| nnodes(mesh)) | ||
| D2 = D1^2 | ||
| solver = Solver(D1, D2) |
There was a problem hiding this comment.
If we go as far as this, it could make sense to add downstream tests to SummationByPartsOperators.jl. I am not sure whether I have documented that D1^ will necessarily return a PeriodicRationalDerivativeOperator. It should be pretty stable, but we use some parts that are less clearly described as public API.
There was a problem hiding this comment.
Ok, I didn't know that PeriodicRationalDerivativeOperator is not quite public. Adding tests to SummationByPartsOperators.jl sounds good. But this can be merged anyway, right?
There was a problem hiding this comment.
The same also holds for FourierPolynomialDerivativeOperator, doesn't it? As far as I can tell, it's also not clearly documented that D1^ will return a FourierPolynomialDerivativeOperator if D1 is a FourierDerivativeOperator.
There was a problem hiding this comment.
Well, I have not added any documentation to it, so it is indeed not really public but more like an implementation detail to make ^ etc. work - which is documented
All of these periodic derivative operators support multiplication and addition such that polynomials and rational functions of them can be represented efficiently
already in the README.md (see https://github.com/ranocha/SummationByPartsOperators.jl?tab=readme-ov-file#periodic-domains).
But we can go ahead and merge this PR here. We don't need to add downstream tests right now, but it may be nice to keep this in mind for the future.
There was a problem hiding this comment.
The simpler option would be to add some explicit tests that ^ returns the appropriate types with some comments that DispersiveShallowWater.jl uses this interface.
There was a problem hiding this comment.
The same also holds for
FourierPolynomialDerivativeOperator, doesn't it? As far as I can tell, it's also not clearly documented thatD1^will return aFourierPolynomialDerivativeOperatorifD1is aFourierDerivativeOperator.
Yes
I added support for
PeriodicRationalDerivativeOperators as second derivative operator andFourierDerivativeOperators for the BBM-BBM and Svärd-Kalisch equations. I also improved the error thrown for an unknown operator.Closes #132.