Remove Float comparison ambiguities from generate_coords
#67
Conversation
JordiManyer
changed the title
generate_coords
| n_vertices = length(unique(model_cell_coords.data)) | ||
|
|
||
| model_coords = fill(VectorValue(fill(T(Inf),Dp)),n_vertices) | ||
|
|
There was a problem hiding this comment.
I would improve the naming convention.
E.g.
topo_cell_ids => cell_corners
model_cell_coords => cell_vertices_coords
model_coords => vertex_coords
model_cell_ids => cell_vertices
topo_coords => corner_coords
|
|
||
| resize!(model_coords,n_vertices) | ||
| topo_coords = (n_vertices == n_corners) ? model_coords : model_coords[1:n_corners] | ||
| return model_cell_ids, model_coords, topo_coords |
There was a problem hiding this comment.
I have a more general question here regarding the need for topo_coords. I understand it is because it is a requirement of the UnstructuredGridTopology constructor.
HOWEVER, if we are separating among topology and geometry, etc. Why do we need the coordinates of the corners in GridTopology??? What am I missing here?
There was a problem hiding this comment.
Also ... how is the coordinate of a corner defined mathematically and why?
Right now, the code is assigning (arbitrarily?) the minimum coordinate among the coordinates of the all vertices that clash into the same corner.
There was a problem hiding this comment.
No idea, it might never be used. But it is required, so we might as well give it something consistent, right?
| if haskey(new_nodes,coord) | ||
| model_cell_ids.data[k] = new_nodes[coord] | ||
| if norm(coord-model_coords[vertex]) > eps(T) | ||
| pos = findfirst(x -> norm(x-coord) < eps(T), model_coords[n_corners+1:n_vertices]) |
There was a problem hiding this comment.
model_coords[n_corners+1:n_vertices] creates a dynamically allocated copy, use view instead
There was a problem hiding this comment.
| if coord != topo_coords[vertex] | ||
| if haskey(new_nodes,coord) | ||
| model_cell_ids.data[k] = new_nodes[coord] | ||
| if norm(coord-model_coords[vertex]) > eps(T) |
There was a problem hiding this comment.
Should we use here the relative error instead of the absolute one?
|
Hi @JordiManyer, Thanks for your work. I have completed a first round of review. Please see my comments. Leaving aside my concern on why do we actually need geometric information for topology-related data (i.e., the coordinates of the corners, whatever they are defined mathematically), my main concern while going over the proposed strategy in this PR was (but no longer is) that we are still comparing numerical values to identify vertices. But I think it is fine, because we only have two different scenarios:
I think that, in order to be even safer, one may use equality instead of Note that Let me know if you agree or not here, or I might be missing something. Finally, on another note, I still wonder myself if I could get some symbolic information out of p4est that allows us to separate vertices from corners, but I guess this requires much more time than the one I have now. UPDATE/CAVEAT: In my reasoning above, I am assuming that p4est is going to return exactly (bit-wise equivalence) the same vertex coordinate for all cells surrounding a vertex. If this is not the case, I think we can force this as a post-process step. |
No description provided.