Skip to content

Improve test coverage #238

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

Merged
merged 29 commits into from
Feb 11, 2025
Merged

Improve test coverage #238

merged 29 commits into from
Feb 11, 2025

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Feb 3, 2025

This pr aims to increase test coverage and fix problems I hit along the way. WIP for now.

Questions:

  • Should Rect() default to Rect(NaN..., 0...) instead of Rect(Inf..., -Inf...)? Otherwise, how should utility functions treat infinite Rects? E.g. should volume return NaN? minimum and maximum? Should we consider -Inf widths as invalid?
  • Does it make sense for mat[Rect2i(1, 1, 2, 2)] to index [2:3, 2:3], i.e. be zero based?

(Significant) (Code) Changes:

  • Rect:
    • refactored constructors to generalize them and make them depend more on each other. (This should only add more constructors)
    • generalized euclidean_distance, update to accept VecTypes
  • added strictly_equal_face_vertices() for comparing the result of split_mesh with pre-merge meshes. This considers two meshes to be equal if a.vertex_data[a.face] == b.vertex_data[b.face] holds for every face and every vertex attribute.
  • removed merge_vertex_indices(faces...[, index_counter]) method which I added as an alternative to merge_vertex_indices(tuple_of_face_arrays[, index_counter]) but never used. This function is internal with the (more) public interface being expand_faceviews(), so this should not be breaking. (It's also not used in MeshIO or Makie)
  • breaking Removed Rect{T}(obj) constructor for boundignboxes as it clashes with the Rect{N, T} definition of the type. The implementations now use Rect{N, T}(obj) with other methods funneling into that one. This enables Rect{N}(obj) and RectT{T}(obj) analogous to other Rect constructors.
  • maybe breaking reworked self_intersections() to allow a segment to intersect with multiple other segments. For this, the format of the returned indices changed from Vector{Int} to Vecto{Tuple{Int, Int}}. This is now an internal change. Also did some low hanging performance optimizations (skip redundant half of the loop)

Test TODO;

  • Rect (file: 57% -> 96%, total: 64% -> 71%)
    • constructors, convert, centered, split, bbox-like constructors
    • euclidean distances
    • Rect as matrix index
    • Matrix - Rect multiplications
  • bits and pieces in basic_types.jl (file: 63% -> 83%, total: 74%)
  • most of triangulation.jl (file: 26% -> 90%, total: 80%)
  • meshes.jl (file: 68% -> 85%, total: 82%)
    • split_mesh
    • duplicate face removal
    • map_coordinates
  • boundingboxes.jl (file: 48% -> 88%, total: 83%)
  • lines.jl (file: 34% -> 98%, total: 86%)
  • spheres.jl (file: 63% -> 100%, total: 86%)
  • OffsetIntegers (file: 61% -> 55% ???) and FixedArrays (unchanged) (total: 86%)

@ffreyer ffreyer marked this pull request as ready for review February 5, 2025 15:06
@asinghvi17
Copy link
Contributor

Do we actually have codecov enabled on this repo? Should we do that, if the coverage is now worth showing off? :D

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 5, 2025

We do, it just doesn't run in prs

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 5, 2025

Well it does, it just errors

Comment on lines 13 to 20
function Rect{N, T}(geom::AbstractGeometry) where {N, T <: Number}
if applicable(Rect{T}, geom)
@warn "`Rect{T}(geom)` is deprecated as the final boundingbox method. Define `Rect{N, T}(geom)` instead."
return Rect{T}(geom)
else
return Rect{N, T}(coordinates(geom))
end
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there is much of a point to making a user facing depwarn here, because on master Rect{T}(geom) only works if T matches the input eltype. I therefore doubt anyone relied on it. It also throws an ArgumentError lower down the line (and would create a stackoverflow with this dep warning.)

For devs I think it makes more sense, because I think it's fairly likely someone would copy the syntax from GeometryBasics. And without a warning, their implementation would silently be ignored.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nvm, doesn't work. I thought for a second that N <: Val was working for catching literals but I don't think it actually does. Without type specifity on Rect{N}(...) this will always call the applicable branch and loop.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 11, 2025

Fixes #155 (if it's not already fixed on master)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 11, 2025

Fixes #207

@ffreyer ffreyer mentioned this pull request Feb 11, 2025
@ffreyer ffreyer requested a review from SimonDanisch February 11, 2025 16:19
@ffreyer ffreyer merged commit 14f3ce4 into master Feb 11, 2025
12 checks passed
@ffreyer ffreyer deleted the ff/test-coverage branch February 11, 2025 16:53
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.

3 participants