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

Multiple arguments #14

Closed
gdalle opened this issue Sep 28, 2023 · 23 comments · Fixed by #16
Closed

Multiple arguments #14

gdalle opened this issue Sep 28, 2023 · 23 comments · Fixed by #16

Comments

@gdalle
Copy link
Collaborator

gdalle commented Sep 28, 2023

How do we include methods in the interface that take several arguments of different types?

@rafaqz
Copy link
Owner

rafaqz commented Sep 28, 2023

Yeah, you would need to define an interface that is specifically for e.g. type a working where type b is the second argument to function f.

It would be cool if you could specify the traits allowed for b.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

At the moment I don't care about traits, but I would like to specify an interface like the following (let's ignore the liberties I took with Interfaces.jl syntax):

@interface AbstractGraph{V} (
    mandatory = (
        vertices(::AbstractGraph),
        edges(::AbstractGraph),
        neighbors(:::AbstractGraph{V}, ::V)
    ),
)

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

See JuliaGraphs/GraphsBase.jl#2

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

We just need to write the macro code to detect that I guess.

But do you only want to know that the methods exist? Or actually compile/run something?

If the second argument accepts Any we wont have proven much without running it.

In the end its hard to beat just using an anonymous function and running the thing.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

If the goal is to have compile-time checking, we are not allowed to run anything, right?

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

The goal is not compile-time checking, thats a pretty limited check of an interface working. E.g. it cant catch half of the OffsetArrays.jl bugs and most other correctness bugs that got me thinking about this in the first place.

While the traits are compile time, the check can go in precompile or tests.

So the fact that the check actually runs is socially enforced rather than compiler enforced. Not perfect of course.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

The trouble is, for some of these functions there is no "default value" to run them on. Typically, the graphs interface requires access functions but no specific constructors

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

You need to provide test objects to run the interface tests.

Or do you mean for the second argument? You could create that from oneunit(V) in your example.

Just make the test objects for additional arguments in the function is the idea.

Maybe we can look at how to do this combinatorially, idk.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

Riiight, so Interfaces.implements is a compile-time trait but it is only based on user declaration, and Interfaces.test is what a user should actually do before declaring that the interface is supported.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

So what needs to change is the syntax for interface declaration and testing.
I guess for testing, instead of a list of objects, one could have a list of named tuples or dictionaries?

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

As for the macro, my first example might be a little hard to achieve in terms of parsing, but this simplified version would already go a long way:

@interface AbstractGraphInterface (
    mandatory = (
        vertices(g::AbstractGraph),
        edges(g::AbstractGraph),
        neighbors(g:::AbstractGraph, v)
    ),
)

Note the difference between the interface name and the type name, cause this is an inheritance-free interface unlike RequiredInterfaces.jl.

Then we could pass something like

Dict(g => MyConcreteGraph(), v => 1)

to the tests?

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

Can we separate out the nice syntax idea and multiple arguments idea? Ultimately the macro needs to generate some anonymous function anyway.

(A macro-free MWE forces us to address how this will actually work - the current macro just copies the code unchanged, there is no magic)

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

I'm confused, to me the new syntax is necessary to accept multiple arguments of different types

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

The output of the @interface macro has to end up being mandatory and optional NameTuples with values of Union{Function,Tuple{Function,...}} where the keys are the trait names.

Those functions get run in the tests for each trait.

So working out the syntax of the final functions is the important part... then we can look at how to map that to a DSL like your example, if its actually necessary.

(Look at the readme examples. They are barely modified by the macro, thats the actual structure thats used in the tests. How do they need to change if we have multiple arguments is the question)

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

I'm sorry I still don't understand what kind of "macro-free example" you want.
If I try to adhere to the Interfaces.jl syntax, I guess a minimal example would look like this:

@interface AbstractGraphInterface (
    mandatory = (
        has_vertex = (g, u) -> has_vertex(g, u) isa Bool,
        has_edge = (g, u, v) -> has_edge(g, u, v) isa Bool,
    ),
    optional = (;)
)

The main question I have is how to impose the type of g since it is not named anywhere in the interface definition.
And ideally, I would also like to make the types of u and v depend on g.

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

Thanks thats what I meant.

If the object is an array or similar we can use u = zero(eltype(g)) in the anonymous function and drop it from the arguments?

I guess there are cases where things like that are not possible and we need a way to pass in the extra arguments to the tests on a trait-by-trait basis (and those will be passed to the anonymous function).

Isnt imposing types on g just g -> g isa SomeType ?

(Types are not specified by default because a lot of interfaces are trait based, so an object being a certain type is just a check you write like any other. We can add syntax for that if it makes things clearer, but its very little code to just write it out as the first condition)

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

If the object is an array or similar we can use u = zero(eltype(g)) in the anonymous function and drop it from the arguments?

That's rather specific, and in our case it wouldn't work cause we want to apply this to arbitrary graphs created by the user, with arbitrary vertex types. So letting the user give the arguments in the test seems like the only way out

Isnt imposing types on g just g -> g isa SomeType ?

In my mind, the interface is linked to a single type, like Duck, or in my case a subtype G <: AbstractGraph{V}, where V is the vertex type. So I wanna specify somehow that the g arguments are those of the type G constrained by the interface

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

That's rather specific

Well I did mention the general solution below that...

So I wanna specify somehow that the g arguments are those of the type G constrained by the interface

All g in these examples are the same object so g -> g isa SomeType is doing exactly what you want already?

In my mind, the interface is linked to a single type,

I dont understand now. The interface is applied to a type in the @implements macro. But the @interface macro doesnt care about types as these days many interfaces are traits based.

I guess we could add a type argument and have the default be Any ?

The you could skip the g -> g isa SomeType.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 29, 2023

All g in these examples are the same objects so g -> g isa SomeType is doing exactly what you want already?

Yes but where is the SomeType coming from? Does it become an additional argument to the interface?

@interface AbstractGraphInterface SomeType (
    mandatory = (
        has_vertex = (g::SomeType, u) -> has_vertex(g, u) isa Bool,
        has_edge = (g::SomeType, u, v) -> has_edge(g, u, v) isa Bool,
    ),
    optional = (;)
)

But in that case the anonymous functions cannot work until the user gives their actual SomeType, in our interface definition it can only be a mute variable

@rafaqz
Copy link
Owner

rafaqz commented Sep 29, 2023

Im so confused... the person that implements the @interface code just writes the trait test out like this or whatever:

istherighttype = g -> g isa SomeType

We can of course add this to the macro as magic, but lets stick to functions here.

Or... are you trying to say you want a check that there are functions defined for exactly some specific type rather than a generic falback?

Otherwise Im lost here

I dont get why you need this:

g::SomeType

If right above it you have

g -> g isa SomeType

These functions in mandatory just run in sequence, so g has to be SomeType

Like:

mandatory = ( 
    istherighttype = g -> g isa SomeType,
    has_vertex = (g, u) -> has_vertex(g, u) isa Bool, 
    has_edge = (g, u, v) -> has_edge(g, u, v) isa Bool, 
),

I guess we are not clear on how the objects are passed to the tests now there are multiple arguments... I was assuming only the extra args are passed in where needed, and g is always just the same object.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 30, 2023

I think it might be a good idea to have a 15-min zoom call one of these days, if you're open to it 😅 this is getting a bit tedious

@rafaqz
Copy link
Owner

rafaqz commented Sep 30, 2023

Its basically always like this lol... the difficulty of these discussions is a lot of why I stopped working on this package, I never experienced anything like it.

Peoples idea of what an interface declaration is/should be is just wildly different.

But Im actually mostly free today, central european time zone.

@gdalle
Copy link
Collaborator Author

gdalle commented Sep 30, 2023

Roadmap

  • Define an Arguments type wrapping a NamedTuple (see Extents.jl)
  • Define a function get_interface_type(a::Arguments) returning the first type of the named tuple
  • Transform any checks in src/test.jl to allow the vector of objects to be either Duck or Argument{(Duck, ...)}
  • Add new tests for multi-argument functions a:Arguments -> f(a.x, a.y)
  • Explain that the first interface test should validate the Argument
  • Add docs

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 a pull request may close this issue.

2 participants