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

Add ValueTuple overload for creating CompressedColumnStorage #46

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

miniksa
Copy link
Contributor

@miniksa miniksa commented Jun 11, 2024

When working with this package, I wanted to use ValueTuple over classic Tuple to avoid the heap allocation that occurs on every single Tuple (improving performance).

@miniksa
Copy link
Contributor Author

miniksa commented Jun 11, 2024

If you want me to add more overloads elsewhere or otherwise deduplicate this somehow... let me know. I intentionally tried to keep this scoped as narrow as possible until hearing from you.

@miniksa
Copy link
Contributor Author

miniksa commented Jun 11, 2024

@Austin-Lamb, my co-worker, suggests that I should use names on the parts of the ValueTuple so it's less ugly than Item1, Item2, etc.

@wo80
Copy link
Owner

wo80 commented Jun 11, 2024

Thanks!

The method parameters should also be documented in the public CompressedColumnStorage class.

Leaving the original conversion method for enumerables of Tuple in place is fine.

I was wondering whether the IEnumerable<Tuple<int, int, T>> EnumerateIndexed() method of the Matrix class should also use ValueTuple. What do you think? It affects the public API, but the change is small, so maybe no need to increment the major version.

/// <summary>
/// Enumerates all values of the matrix.
/// </summary>
/// <returns>Enumeration of tuples (i, j, a[i, j]).</returns>
public abstract IEnumerable<Tuple<int, int, T>> EnumerateIndexed();

@miniksa
Copy link
Contributor Author

miniksa commented Jun 11, 2024

Thanks!

The method parameters should also be documented in the public CompressedColumnStorage class.

Leaving the original conversion method for enumerables of Tuple in place is fine.

I was wondering whether the IEnumerable<Tuple<int, int, T>> EnumerateIndexed() method of the Matrix class should also use ValueTuple. What do you think? It affects the public API, but the change is small, so maybe no need to increment the major version.

/// <summary>
/// Enumerates all values of the matrix.
/// </summary>
/// <returns>Enumeration of tuples (i, j, a[i, j]).</returns>
public abstract IEnumerable<Tuple<int, int, T>> EnumerateIndexed();

Yeah I thought about that, but I didn't want to change the public API. I almost thought of making EnumerateIndexedAsValueTuples() or something just to not break anyone.... but that depends what you prefer.

I know that it would increase the performance for when it's used in the FromMatrix() method and I could link that up without all those temporary allocations.

…and CompressedColumn storage mechanisms derived from that class. Use the value-based variant in the OfMatrix helper to avoid allocations.
@miniksa
Copy link
Contributor Author

miniksa commented Jun 11, 2024

Try this on, @wo80. I made a separate abstract method and implemented that one in both the Compressed and Dense matrix classes, then used it in the .OfMatrix helper. Happy to keep iterating if you want me to just replace the old one. I'm just normally allergic to public API breaking changes.

@wo80
Copy link
Owner

wo80 commented Jun 11, 2024

I'm just normally allergic to public API breaking changes.

I fully agree. Version 4 was just published, and I don't want to release a new major version with such a minor change, so let's leave it this way.

I'll add a TODO comment to replace the old method and hopefully will remember when version 5 is released ... in 10 years or so 😁

@wo80 wo80 merged commit f145e93 into wo80:master Jun 12, 2024
1 check 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.

2 participants