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

Use index types to number polynomials #6

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

timholy
Copy link
Collaborator

@timholy timholy commented Mar 23, 2024

This is a substantial API change, although it is not a
breaking change as the old API is still supported via
deprecations. The concept is to use three types, NM,
OSA, and Noll, to specify the numbering
convention. The constructor of each checks for
validity, so it is impossible to construct invalid index
values. Then, instead of passing in both an ::Int and
an index ∈ (:OSA, :Noll) to lots of functions, the
package functions are simply implemented in terms
of their preferred index type, and other index types
are supported by conversion.

The advantages of this design are:

  • index validity is guaranteed and only has to be checked in one place
  • indices are "self-documenting" about how they should be interpreted (unlike an Int).
  • using functions named x2y for conversion means that if you have n different indexing conventions, you need n^2-n (quadratic in n) exported names. But if you use types, you only need n exported names (linear in n).

This also enhances flexibility, e.g., on master specifying J = 0:9 for Zernikecoefficients results in an error because that function only takes a Vector{Int}. It also adopts the conventions of the Julia style guide regarding capitalization of names.

Finally, to ensure I didn't make breaking changes, I deliberately left the tests the way they were: my editor cleaned up some extraneous spaces, and I added one convert test, but all the old tests still pass. If you like this and want to merge it, in a separate PR I will:

  1. move the old tests to a test/deprecated.jl file
  2. create their modern analogs using the new API

Some day you can release v0.2 (or v1.0?) of this package, at which point you can strip out all the old deprecations. But having them in place should provide very useful hints for people about how to migrate over to the new API.

CC @jona125

This is a substantial API change, although it is not a
breaking change as the old API is still supported via
deprecations. The concept is to use three types, `NM`, `OSA`, and `Noll`, to specify the numbering
convention. The constructor of each checks for
validity, so it is impossible to construct invalid index
values. Then, instead of passing in both an `::Int` and
an `index ∈ (:OSA, :Noll)` to lots of functions, the
package functions are simply implemented in terms
of their preferred index type, and other index types
are supported by conversion.

The advantages of this design are:
- index validity is guaranteed and only has to be checked in one place
- indices are "self-documenting" about how they should be interpreted (unlike an `Int`).
@timholy
Copy link
Collaborator Author

timholy commented Mar 23, 2024

If you want to see what this looks like, check the modified README. You can view it by changing branches on the main page, but here's a link: https://github.com/timholy/ZernikePolynomials.jl/tree/teh/index_types

@rdoelman
Copy link
Owner

Your contribution is a significant improvement of the package. Thanks!

@rdoelman rdoelman merged commit ff0bb98 into rdoelman:master Apr 13, 2024
3 checks passed
@timholy timholy deleted the teh/index_types branch April 16, 2024 10:48
@timholy
Copy link
Collaborator Author

timholy commented Apr 16, 2024

Great, glad you like it! I've updated the tests to the new API in #7.

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.

2 participants