[WIP][SetParameters] Generalize to work for any type, rename to TypeParameterAccessors

[kmp5VT]

Description

Rename the SetParameters package and utilize the type information provided by DataType and modify the package to use Base functions to change/access the parameters.

Checklist:

• Rework the code to use base functions
• Verify that the code works with equivalent speed as previous implementation
• Verify type stability of the code
• Rename the module per [SetParameters] Rename SetParameters to TypeParameterAccessors #1327

[kmp5VT]

@mtfishman Right now the new code compiles but I have not verified that tests pass. I will be working on this more today. I did some timing and see that the new code you wrote is roughly the same speed or faster than the previous implementation and uses slightly fewer allocations.

[kmp5VT]

So far I have the get_parameters working and, as I have said in an earlier comment, I am now working on the set_parameter functions. I realize that the DefaultParameter system will have to be rewritten to work in the new system but that should be too hard. I have added a comment to help remember.

[mtfishman]

Suggested change

	Base.ndims(type::Type{<:BlockSparse}) = parameter(type, Base.ndims)
	Base.ndims(type::Type{<:BlockSparse}) = parameter(type, ndims)

[mtfishman]

Can't this just be a generic definition:

Base.ndims(type::Type) = parameter(type, ndims)

for any type?

[kmp5VT]

I don't think this is possible because BlockSparse is a AbstractArray so if we don't define Base.ndims(type::Type{<:BlockSparse) then it will use the AbstractArray version.

[mtfishman]

Oh right, never mind. I was forgetting that would be type piracy, since Julia already defines Base.ndims(::Type{<:AbstractArray}).

[mtfishman]

See my comment above, we may want to define a TypeParameterAccessors.ndims function.

[mtfishman]

I'm still confused by this function call. What if parameter_names(arraytype) doesn't correspond with the output of parameters(xs)? For example, xs could just have an element type parameter, in which case it just outputs something like (Float64,), but parameter_names(arraytype) outputs (eltype, ndims, storagemode).

It seems like this should be:

  arraytype_specified = specify_parameters(
    arraytype, parameter_names(xs), parameters(xs)
  )

instead.

[kmp5VT]

@mtfishman So I think I need to go through all of the code and find where ndim and eltype among other things are defined and modify the code to use the new system.

[kmp5VT]

@mtfishman The changes from today address the ndims problem we discussed. I have not yet had time to address what we went over in the meeting today. Namely moving all of the Base functions to their own file and migrating the default_parameter system to be called via a Position instead of a name. I will look into these things tomorrow.
As a reminder to myself, we also discussed keeping with storagetypestoragemode and moving the definition to work on AbstractGPUArray, moving the unwrap_array_type function to TypeParameterAccessors and renaming the Unwrap module as ExposeArrayWrappers. Please add if I forgot any task

[mtfishman]

Sounds like a good summary, thanks.

[mtfishman]

@kmp5VT regarding the comment Miles made about how it is a bit strange for users to get TypeVar outputs from functions like parameter, a suggestion there would be to keep that behavior of parameter since it is useful internally for determining if parameters are specified or not, but then define a stricter function get_parameter[s] that errors if you try to access any unspecified types (like how Base.ndims in Julia works right now).

For example, it could be defined like this:

is_specified_parameter(param::TypeVar) = false
# Handles both `DataType`, or an `isbits` type parameter.
is_specified_parameter(param::Any) = true

function get_parameter(type::Type, pos)
  param = parameter(type, pos)
  !is_specified_parameter(param) && error("The requested type parameter isn't specified.")
  return param
end

With that kind of design, if we ever decided to change our design or convention for what parameter outputs if a type parameter isn't specified (for whatever reason), that choice could be handled by is_specified_parameter. We could also define:

is_parameter_specifed(type::Type, pos) = is_specified_parameter(parameter(type, pos))

which I think would be generally useful. I'm not sure if it is a good idea to name both is_specified_parameter(type::Type, pos) and is_specified_parameter(param) the same thing but I can't think of better names for those. I changed the proposal to name one of them is_parameter_specifed and the other is_specified_parameter.

Then, we should make definitions like parenttype(type::Type) = get_parameter(type, parenttype), instead of using parameter, since those functions should error if there is no specified parenttype.

[mtfishman]

What's the reasoning behind this change?

[mtfishman]

I'm not sure we really need the SpecifiedParameter trait, I found it was sufficient for type stability just to do a runtime check of is_parameter_specified, see https://gist.github.com/mtfishman/c3859d89f0d0c710f5a4cb11a782ae0c, and would be a lot simpler.

[mtfishman]

Suggested change

	Base.Int(p::Position) = parameter(p)
	Base.Int(p::Position) = Int(parameter(p))

to make sure it really outputs Int.

[mtfishman]

Suggested change

	return error("There does not yet exist a name for the type $(type) at position $(int(p))")
	return error("There does not yet exist a name for the type $(type) at position $(Int(p))")

[mtfishman]

I'm guessing this should be removed.

[mtfishman]

Here's a suggestion for avoiding this code duplication across tests. We can define our own macro:

using Test

macro inferred_above(version, expr)
  quote
    if VERSION ≥ $(version)
      @inferred $(expr)
    else
      $(expr)
    end
  end
end

f(x) = x > 0.5 ? Int : Float64
@inferred_above v"1.7" f(0.2)
@inferred_above v"1.10" f(0.2)

which only tests inference if the version is at or above a certain value. So then we can use all of the same tests, and adjust the version as needed if we know inference fails on older versions but we don't care if that's the case.