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.

Sign up for GitHub

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

corrected typos and added missing description of GenericQuadrature #764

Merged
merged 1 commit into from
Mar 11, 2022
Merged

corrected typos and added missing description of GenericQuadrature #764

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ReferenceFEs/Quadratures.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ end
struct GenericQuadrature{D,T} <: Quadrature{D,T}
coordinates::Vector{Point{D,T}}
weights::Vector{T}
name::String
end
"""
struct GenericQuadrature{D,T} <: Quadrature{D,T}
Expand All @@ -99,7 +100,7 @@ get_weights(q::GenericQuadrature) = q.weights
get_name(q::GenericQuadrature) = q.name

function GenericQuadrature(a::Quadrature)
GenericQuadrature(get_coordinates(q),get_weights(a),get_name(q))
GenericQuadrature(get_coordinates(a),get_weights(a),get_name(a))
Copy link
Member

Choose a reason for hiding this comment

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

Hi @kishore-nori. Thanks for your changes. How can it be that this was working even with these typos? I guess that this particular part of the code was not being tested, agreed? Perhaps you may also add a test to the test suite at the appropiate place to test this particular scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes @amartinhuertas, I was surprised too, I think the code wasn't specialising to this method, since only standard Quadrature was being used and it might have never happened so that a Quadrature which is not GenericQuadrature was passed through it in the tests or examples till now. Yes.. I ll think of a test for testing this in the ReferenceFEsTests/QuadratureTests.jl.

Copy link
Collaborator Author

@kishore-nori kishore-nori Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Quadrature has only one concrete subtype which is the GenericQuadrature and Quadrature is an abstract type. I am not exactly sure how to come up with a test for it. Currently, if evaluated it using get_coordinates, get_weights and get_name it would result in @abstractmethod call as a fall back.

If I add a new concrete subtype DummyQuadrature similar to GenericQuadrature, and run test_quadrature it would result in the @abstractmethod call resulting in an ERROR: This function belongs to an interface definition and cannot be used. which although verifies the working of the function and if free from typos specific errors but results in an error where the test wouldn't pass.

Does Gridap follow a generic idea for testing abstract types? Would be instructive for me to have such example..

Copy link
Member

Choose a reason for hiding this comment

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

ok. I see what you mean. We can accept the PR like now. But keep in mind that we may add a test once we have more subtypes of GenericQuadrature as far as these do not overload the GenericQuadrature constructor with an specialized version.

end

function GenericQuadrature(a::GenericQuadrature)
Expand Down Expand Up @@ -141,4 +142,3 @@ function Quadrature(p::Polytope,degree)
end
quad
end