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

changed vectors&matrices file, userguideUpdate branch #72

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lvllvl
Copy link

@lvllvl lvllvl commented Aug 23, 2022

Updated user guide based on Issue 988 @avl

This does run on my system, sorry I didn't compile the previous PR. This one does work on my end.

Copy link
Sponsor Contributor

@Andlon Andlon left a comment

Choose a reason for hiding this comment

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

Thanks, this is a very useful addition! I think perhaps matrix! and vector! should become before dvector! and dmatrix! though? The ordering right now seems a bit arbitrary, especially since point! appears between matrix! and vector!.

I personally believe that we should mention the macros already in the Matrix construction section, because they should probably be preferred to ::new, which is currently advocated in that section. However, we can save this for a separate PR if you don't want to add more work to this one.

Otherwise see comments for some minor points.

@@ -501,3 +501,69 @@ compile-time.
![Fixed resizing](/img/resizing.svg)

</div>

## Matrix Macros
### Macros
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I think maybe we should remove hte ### Macros header because it just causes unnecessary nesting in the navigation (on the right side).

Copy link
Author

Choose a reason for hiding this comment

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

Ok, removed the header.

use nalgebra::matrix;
// Produces a Matrix3<_> == SMatrix<_, 3, 3>
let a = matrix![ 1, 2, 3;
4, 5, 6;
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

This is not properly aligned

Copy link
Author

Choose a reason for hiding this comment

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

Updated:

I updated the spacing between paragraphs to try and match the rest of the document. Hopefully I understood correctly.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Sorry, I was not clear: I meant that the matrix elements are not aligned, e.g. the 4 appears one character to the left of 1, which looks odd. Please remove the new line after the comment again!

## Matrix Macros
### Macros
#### dmatrix!
Construct a dynamic matrix directly from data. The syntax is exactly the same as for matrix!, but instead of producing instances of SMatrix, it produces instances of DMatrix.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Maybe use code blocks for vectors, types etc, so that it's consistent with the rest of the user guide. This applies throughout.

Copy link
Author

Choose a reason for hiding this comment

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

Updated: Ok I tried to make sure and use code blocks for types, vectors, etc. so I could match the rest of the document.

@lvllvl lvllvl requested a review from Andlon September 10, 2022 02:40
Copy link
Sponsor Contributor

@Andlon Andlon left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me, with the exception of one tiny thing, see comment. I was not so clear in my feedback which resulted in a very slight misunderstanding. Would be great to fix that last remaining issue, as it'll look much more polished in the user guide.

Otherwise, can you please confirm that everything renders correctly in the final version?

use nalgebra::matrix;
// Produces a Matrix3<_> == SMatrix<_, 3, 3>
let a = matrix![ 1, 2, 3;
4, 5, 6;
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Sorry, I was not clear: I meant that the matrix elements are not aligned, e.g. the 4 appears one character to the left of 1, which looks odd. Please remove the new line after the comment again!

@lvllvl
Copy link
Author

lvllvl commented Sep 12, 2022

Ok sounds good, thanks for clarifying. I made the changes on the matrix! code sample.

I also ran the Docusaurus site and it rendered properly on my end.

I also removed the new line after the comment.

@lvllvl lvllvl requested a review from Andlon September 12, 2022 23:29
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.

None yet

2 participants