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

Update System.Numerics.Vector<T> documents #3334

Merged
merged 5 commits into from Jan 6, 2020
Merged

Conversation

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Oct 12, 2019

Summary

  • Emphasise that this type is not appropriate for general purpose vector operations
  • Update ctor remarks to emphasise only primitive numeric types are allowed
  • Update ctor remarks to emphasise only the first Vector<T>.Count elements are added to the vector (for Span/Array overloads)
  • Add information about exception that can be thrown when array.Length < Vector<T>.Count

Fixes dotnet/corefx#23448

cc @tannergooding (Issue author, area owner)

@Gnbrkm41 Gnbrkm41 changed the title Update System.Numerics.Vector documents Update System.Numerics.Vector<T> documents Oct 12, 2019
@Gnbrkm41

This comment has been minimized.

Copy link
Contributor Author

Gnbrkm41 commented Oct 12, 2019

Looking at some other issues, it's not clear whether the constructors throwing IndexOutOfRangeException is intentional or not: dotnet/corefx#25344

@Gnbrkm41

This comment has been minimized.

Copy link
Contributor Author

Gnbrkm41 commented Oct 12, 2019

dotnet/corefx#25344 (comment)

Yes, it is fine to document the current behavior for exception thrown by Vector constructors. Vectorconstructors do not follow the framework design guidelines for performance reasons. Another difference from the standard is that they throw NullReferenceException (vs. the ArgumentNullException recommended by the framework design guidelines).

Gnbrkm41 added a commit to Gnbrkm41/dotnet-api-docs that referenced this pull request Oct 12, 2019
* Pull the type constraint to the top of the ctor docs since it's shared throughout, list out the types for future-proofing
* Update param descriptions for array/spans to be explicit about the remainders

Part of dotnet-api-docs/dotnet#3334

Co-Authored-By: Tanner Gooding <tagoo@outlook.com>
@Thraka Thraka requested a review from tannergooding Oct 17, 2019
@carlossanlop

This comment has been minimized.

Copy link
Member

carlossanlop commented Dec 12, 2019

@tannergooding can you help answer @Gnbrkm41 's question?

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Dec 12, 2019

I think use of the word "primitive" is fine, we just need to ensure that its meaning and/or constraints is defined somewhere

Gnbrkm41 and others added 3 commits Oct 12, 2019
* Emphasise that this type is not appropriate for general purpose vector operations
* Update ctor remarks so that only primitive numeric types are allowed
* Update ctor remarks so that only the first `Vector<T>.Count` elements are added to the vector (for Span/Array overloads)
* Add information about exception that can be thrown when array.Length < Vector<T>.Count

Related: dotnet/corefx#23448
* Pull the type constraint to the top of the ctor docs since it's shared throughout, list out the types for future-proofing
* Update param descriptions for array/spans to be explicit about the remainders

Part of dotnet-api-docs/#3334

Co-Authored-By: Tanner Gooding <tagoo@outlook.com>
* Reference the type instead of just writing it out
@Gnbrkm41 Gnbrkm41 force-pushed the Gnbrkm41:patch-1 branch from 196c57d to d3fc6b2 Dec 14, 2019
@Gnbrkm41

This comment has been minimized.

Copy link
Contributor Author

Gnbrkm41 commented Dec 14, 2019

I've added some explanations on what we mean when we say "primitive", though I don't think it's a problem anymore given we only have 3 usage of the word "primitive" now.

@Thraka

This comment has been minimized.

Copy link
Contributor

Thraka commented Dec 16, 2019

I'm not seeing the emphasis about what you said:
- Emphasis that this type is not appropriate for general purpose vector operations

Can you clarify that? So I see that there is this clause:

>It is intended to be used as a building block for vectorizing large algorithms, and therefore cannot be used directly as an arbitrary length vector or tensor.

But that doesn't really tell me to not use it. It says it's intended for X, but code compiles and runs when you use it as a length vector, right? I've used it for an `intpoint` class or a `floatpoint` class, but I don't get the sense of why I'm doing something wrong and what my alternative is.

Scratch what I said above. I was really thinking of Vector2. Does Vector2 fall into this same category of "you shouldn't really use this for everyday normal programming?"

@Gnbrkm41

This comment has been minimized.

Copy link
Contributor Author

Gnbrkm41 commented Dec 17, 2019

Does Vector2 fall into this same category of "you shouldn't really use this for everyday normal programming?"

I don't think it does. Vector2/3/4 are types that represent the mathematical definition of Vector and definitely can be used in places where you need them, like when you represent points on a 2D space 🙂

It's just that Vector can cause confusion that it exists for the same reason (representing mathematical vector) when it's not due to its name and it being in the same System.Numerics namespace, when it really is for vectorisation of algorithms.

@Thraka
Thraka approved these changes Dec 17, 2019
@Gnbrkm41

This comment has been minimized.

Copy link
Contributor Author

Gnbrkm41 commented Dec 17, 2019

While I did end up changing the singular to plural with the last commit, I'm actually not entirely sure which one is correct; As a non-native English speaker this is one of those things that I honestly never thought much about, but this made me wonder which is supposed to be used.

Asking around a few people the result I got was to use singular, which contradicts with the suggestion I've got here (which obviously does not help). It would be nice if we could get more opinion or a definitive answer on this...

Apart from that, I probably should additionally state that the Count is a property because it can be confusing to read.

@carlossanlop

This comment has been minimized.

Copy link
Member

carlossanlop commented Dec 17, 2019

Thank you for working on this, @Gnbrkm41 .
@Thraka has signed off, and the build is passing without warnings.
@tannergooding do the PR looks good to merge?

@carlossanlop carlossanlop merged commit 7afedf0 into dotnet:master Jan 6, 2020
6 checks passed
6 checks passed
OpenPublishing.Build Validation status: passed
Details
OpenPublishing.Build (1 of 3) Waiting for processor completed at 01:47:48 PST
OpenPublishing.Build (2 of 3) Preparing completed at 02:01:06 PST
OpenPublishing.Build (3 of 3) Building completed at 02:30:52 PST
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.