Skip to content

Add output_size #413

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

Closed
wants to merge 1 commit into from
Closed

Add output_size #413

wants to merge 1 commit into from

Conversation

visr
Copy link

@visr visr commented Mar 24, 2025

I wanted to update SparseConnectivityTracer to DataInterpolations v8 in adrhill/SparseConnectivityTracer.jl#234 but got a bit confused.

#396 removes the output dimension type parameter and the old get_output_dim. Even though it talks about dimensions it was really the size of the output.

SparseConnectivityTracer was one of the reasons output_dim was added to the API, but this actually was the ndims of the output (perhaps output_ndims would be a better name). But that is not what SparseConnectivityTracer needs. Hence I am proposing to add output_size, such that this code doesn't have to live in SparseConnectivityTracer.

@ChrisRackauckas
Copy link
Member

length should be the number of points and size the size of an output?

@visr
Copy link
Author

visr commented Mar 25, 2025

Sounds reasonable, but not sure what your comment means for this PR.

@visr visr marked this pull request as draft March 25, 2025 12:49
@visr
Copy link
Author

visr commented Mar 25, 2025

Converted to draft until we are sure this is what SparseConnectivityTracer really needs, see adrhill/SparseConnectivityTracer.jl#234 (comment).

@visr visr marked this pull request as ready for review April 8, 2025 07:25
@visr
Copy link
Author

visr commented Apr 8, 2025

It is clear now that this would be needed to finish adrhill/SparseConnectivityTracer.jl#234 properly.

Copy link
Member

@SouthEndMusic SouthEndMusic left a comment

Choose a reason for hiding this comment

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

Looking good

@visr visr closed this Apr 8, 2025
@SouthEndMusic
Copy link
Member

SouthEndMusic commented Apr 8, 2025

Something odd happened here: I merged this but it displays as closed 🤔 @visr didn't close the PR. When merging there was an error message saying something like "the server could not be reached".

@visr visr deleted the output_size branch April 8, 2025 09:43
@ChrisRackauckas
Copy link
Member

😅 Just make a new PR?

@visr
Copy link
Author

visr commented Apr 8, 2025

No need, despite the red closed icon, it is on master as intended.

SebastianM-C pushed a commit to SebastianM-C/DataInterpolations.jl that referenced this pull request Apr 9, 2025
oscardssmith added a commit that referenced this pull request Apr 30, 2025
we need a new version for #413
@oscardssmith oscardssmith mentioned this pull request Apr 30, 2025
ChrisRackauckas pushed a commit that referenced this pull request Apr 30, 2025
we need a new version for #413
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