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

Changed input style for add_polygon! #119

Merged
merged 8 commits into from
Oct 9, 2024

Conversation

TatjanaWeiler
Copy link
Contributor

Easier handling for add_polygon! function, now consistent with input style of add_box! function,
local test worked but GMT integration test failed, which is at the moment also the case for the normal version

Changed input style  for add_polygon! Now it is the same as for add_box !
@@ -593,7 +593,7 @@ end


"""
add_polygon!(Phase, Temp, Grid::AbstractGeneralGrid; xlim::Vector(), ylim=Vector(2), zlim=Vector(), phase = ConstantPhase(1), T=nothing, cell=false )
add_polygon!(Phase, Temp, Grid::AbstractGeneralGrid; xlim=Tuple{}, ylim=Tuple{2}, zlim=Tuple{}, phase = ConstantPhase(1), T=nothing, cell=false )

Choose a reason for hiding this comment

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

What is the point of Tuple{} and Tuple{2}? If you want to make empty tuples you can to just xlim=()

julia> Tuple{} == ()
false

@@ -634,10 +634,20 @@ julia> write_paraview(Model3D,"LaMEM_ModelSetup") # Save model to para

"""
function add_polygon!(Phase, Temp, Grid::AbstractGeneralGrid; # required input
xlim::Vector=[], ylim::Vector=[], zlim::Vector=[], # limits of the box
xlim=Tuple{}, ylim=Tuple{2}, zlim=Tuple{}, # limits of the box

Choose a reason for hiding this comment

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

Same as above, these are not empty tuples.

phase = ConstantPhase(1), # Sets the phase number(s) in the box
T=nothing, cell=false ) # Sets the thermal structure (various functions are available)


xlim = collect(xlim)
Copy link
Member

@albert-de-montserrat albert-de-montserrat Jun 1, 2024

Choose a reason for hiding this comment

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

Here you are changing the type of xlim from whatever it is originally to a Array. This yields a type instability, you could fix it just by changing the name of the xlim either in the left hand side or the right hind side of the expression. Same for ylim and zlim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments. I changed the names of xlim, ylim and zlim.
I have not changed the input style because I used the same as for add_box and all the other add function. Should I change that everywhere? And should I also change that in the help functions?

Choose a reason for hiding this comment

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

I have not changed the input style because I used the same as for add_box and all the other add function. Should I change that everywhere? And should I also change that in the help functions?

It would be a good idea to change everything that looks like that indeed. I have the suspicion that all/most those functions will crash if the keyword arguments are not provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many functions need a Tuple with a specific length and not an empty one. Would e. g. Tuple{Float64, Float64} be better even though at the moment Int64 and Float64 are possible or is it better to place everywhere empty tuples and define it only in the help function what kind of input is needed?

Choose a reason for hiding this comment

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

The problem is the following. If you do, for example,

x=Tuple{2}

you can see that x is not a actual usable tuple. x is actually a type if you define it that way.

julia> typeof(x)
DataType

This means that if the user calls any of these functions without explicitly passing x into the function, the function will crash because x is not an actual tuple. I guess this is what you want to do:

julia> function foo(; x::NTuple{2, T} = (1,1)) where T
           return x
       end
foo (generic function with 1 method)

Now you can see how this works depending on what I pass as x

julia> foo(; x=(1,1))
(1, 1)

julia> foo()
(1, 1)

julia> foo(; x=(1.0, 1.0))
(1.0, 1.0)

julia> foo(; x=(1.0, 1.0, 1.0))
ERROR: MethodError: no method matching var"#foo#5"(::Tuple{Float64, Float64, Float64}, ::typeof(foo))

Closest candidates are:
  var"#foo#5"(::Tuple{T, T}, ::typeof(foo)) where T
   @ Main ~/Desktop/DevPkgs/GeophysicalModelGenerator.jl/volcano.jl:70

Stacktrace:
 [1] top-level scope
   @ ~/Desktop/DevPkgs/GeophysicalModelGenerator.jl/volcano.jl:77

if you define x like that in the function call, this means that it can be a Tuple{T, T} where T Any, but in the right hand side you need to provide a default value for x; in this case x=(1,1), but could be whatever other tuple.

Hope this makes any sense!

Changes of the functions in the test files
@albert-de-montserrat
Copy link
Member

@TatjanaWeiler could you trigger the CI again?

@boriskaus boriskaus merged commit 4dcf238 into JuliaGeodynamics:main Oct 9, 2024
16 checks passed
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