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

Fix output dimensions type parameter #398

Closed
wants to merge 3 commits into from
Closed

Conversation

devmotion
Copy link
Member

Same as #396 (ie changes N in AbstractInterpolation{T,N} to refer to ndims(interp(x)) for a single input x) but keeps N as a type parameter.

@devmotion
Copy link
Member Author

devmotion commented Feb 20, 2025

The test failures (or rather what they reveal that I missed so far) makes me wonder whether keeping N as a type parameter is actually a good idea: The T in AbstractInterpolation{T, N} is actually not referring to the element type of the output which I had assumed (so that an output value of e.g. Vector{Float64} would be reported as AbstractInterpolation{Float64, 1} and scalar Float64 as AbstractInterpolation{Float64, 0}) but actually is the type of the output (or more precisely of the u provided to the constructor), i.e., a Vector{Float64} output results in a AbstractInterpolation{Vector{Float64}, 1}}.

So the N seems quite redundant since it is already part of T? So maybe a trait - that could even be based on AbstractInterpolation{T} directly instead of accessing interp.u - would actually be the better choice? What do you think @ChrisRackauckas ?

@ChrisRackauckas
Copy link
Member

This was for DI, so @adrhill what is best here?

@adrhill
Copy link

adrhill commented Feb 23, 2025

I'm happy to adapt to anything you decide upon.

It would be practical for me to have a stable API that allows me to determine the output shape based on an AbstractInterpolant and an input.

@devmotion
Copy link
Member Author

an input.

An input? The API discussed so far, regardless whether based on a type parameter or trait, would return ndims of the output for a single input. Multiple inputs, however, could be stacked or not, this is not determined by this type parameter or trait.

@adrhill
Copy link

adrhill commented Feb 23, 2025

Exactly, which is why I would need both the AbstractInterpolant{…} and the input to infer the output shape. Or is there something I‘m missing?

@devmotion
Copy link
Member Author

It's not clear to me what you actually need. Based on your last comments everything discussed so far seems useless?

@adrhill
Copy link

adrhill commented Feb 25, 2025

My initial request, which now doesn't matter anymore, was to mirror array types, but you've shown me in #396 that my assumptions on DataInterpolations' input and output types were wrong.

I've therefore reapeatedly stated that I have no qualified opinions on DataInterpolations' type design and that N can be removed.

What would be helpful for SCT is a stable API that allows me to determine the output shape based on an interpolant (whatever the type ends up looking like) and an input. This is needed since SCT's global tracers can't evaluate the actual call to _interpolate.

@ChrisRackauckas
Copy link
Member

Alright, so then if no one needs it, let's just remove it then? Are we all agreed? I just want to do whatever makes people happy, I don't care about this parameter and it was made for you and others 😅

@sathvikbhagavan
Copy link
Member

Yes, I am fine with removing it.

@adrhill
Copy link

adrhill commented Feb 26, 2025

Sorry by the way, I did not expect this to cause so much trouble.

@ChrisRackauckas ChrisRackauckas deleted the dw/output_ndims branch March 1, 2025 18:54
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.

4 participants