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

CooMatrix::reserve added #885

Merged
merged 2 commits into from May 9, 2021
Merged

Conversation

chammika-become
Copy link
Contributor

PR for #881

  • Added doc tests as well.

Note: I've noticed missing doc-tests in some of the APIs. Can add them in future PRs if it helps the documentation.

Copy link
Sponsor Collaborator

@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 for this PR, @chammika-become! Looks good to me, and exactly as intended in #881. I only noticed a couple of very minor grammatical things in the doc comments (I hope I got it correct myself, my English is not flawless).

I think the docs would definitely benefit from some additional doc tests, yes! Would be happy to see additional PRs if you would like to work on this :)

/// Reserves capacity for COO matrix by at least `additional` elements.
///
/// This increase the capacities of triplet holding arrays by reserving more space to avoid
/// frequent reallocations, in `push` operations.
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

maybe remove the comma here? ("... frequent reallocations in push operations)

///
/// ## Panics
///
/// Panics if any of the individual allocation of triplet arrays fail.
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

Perhaps "Panics if any of the individual allocations of triplet arrays fails."

/// ```
/// # use nalgebra_sparse::coo::CooMatrix;
/// let mut coo = CooMatrix::new(4, 4);
/// // Reserver capacity in advance
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

Reserver -> Reserve?

@chammika-become
Copy link
Contributor Author

Thank you for the corrections. I will pay more attention to grammar next time.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented May 4, 2021

No worries, as I said they were super minor, and it wouldn't really have been necessary to fix them either, because the meaning was clear. I just prefer fixing them up front, because nobody will bother to fix it later!

@sebcrozet
Copy link
Member

Thanks!

@sebcrozet sebcrozet merged commit d67aec8 into dimforge:dev May 9, 2021
@Andlon Andlon mentioned this pull request May 20, 2021
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

3 participants