Add dependency on SPGrid #22109

xuchenhan-tri · 2024-11-01T00:45:46Z

This change is

xuchenhan-tri

@jwnimmer-tri this is the implementation of my understanding from our discussion about adding SPGrid as a dependency.
Can you take a look when you have some spare time (or delegate to another build expert if spare time is too rare for you in Q4)?
BTW, I haven't disabled macOS in this PR yet so that you can get a flavor of the failure.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

xuchenhan-tri · 2024-11-05T18:43:13Z

cc @amcastro-tri fyi

xuchenhan-tri

-(status: do not review)

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

jwnimmer-tri

The build system is basically on track. I posted some comments inline below.

Reviewed 18 of 20 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

tools/workspace/spgrid_internal/BUILD.bazel line 2 at r1 (raw file):

# This file exists to make our directory into a Bazel package, so that our
# neighboring *.bzl file can be loaded elsewhere.

nit Please remove this whole comment. We don't copy-paste this boilerplate anymore on master. I imagine you started this work from an older branch.

tools/workspace/spgrid_internal/package.BUILD.bazel line 12 at r1 (raw file):

    include_prefix = "SPGrid/Core",
    # TODO(xuchenhan-tri): Enable Haswell on builds that support it.
    # TODO(xuchenhan-tri): Disable macOs.

typo

Suggestion:

macOS

multibody/mpm/BUILD.bazel line 9 at r1 (raw file):

package(default_visibility = ["//visibility:public"])

nit Somewhere (maybe here) we should have a comment that when we enable SPGrid in our releases, then at the same time we also need to install its license file:

drake/tools/workspace/BUILD.bazel

Line 108 in f859518

"spdlog",

third_party/spgrid/LICENSE line 1 at r1 (raw file):

//#####################################################################

BTW To confirm -- the files in this directory are (currently) 100% original, unaltered from upstream, no changes from you (yet)?

third_party/spgrid/LICENSE line 1 at r1 (raw file):

//#####################################################################

nit If possible (and I can help if you like), it would be better to have this PR be two "properly curated" commits where the first commit is only the third_party/spgrid code (sans README), and the second commit is all of the other Drake changes to wrap it. The first commit would have Co-Authored-By: ... lines in its body which cite the original author(s) and/or paper(s).

xuchenhan-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/BUILD.bazel line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Somewhere (maybe here) we should have a comment that when we enable SPGrid in our releases, then at the same time we also need to install its license file:

drake/tools/workspace/BUILD.bazel

Line 108 in f859518

"spdlog",

Done

third_party/spgrid/LICENSE line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW To confirm -- the files in this directory are (currently) 100% original, unaltered from upstream, no changes from you (yet)?

I made one change: in their license file, it originally says

because it was an anonymous submission to a double blind conference/journal,

I modified it to

to reflect the actual paper title

third_party/spgrid/LICENSE line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If possible (and I can help if you like), it would be better to have this PR be two "properly curated" commits where the first commit is only the third_party/spgrid code (sans README), and the second commit is all of the other Drake changes to wrap it. The first commit would have Co-Authored-By: ... lines in its body which cite the original author(s) and/or paper(s).

Working. I modified the commit history. Still need to find author information to fill in.

tools/workspace/spgrid_internal/BUILD.bazel line 2 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Please remove this whole comment. We don't copy-paste this boilerplate anymore on master. I imagine you started this work from an older branch.

Done

tools/workspace/spgrid_internal/package.BUILD.bazel line 12 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

typo

Done, I fixed the TODO too, PTAL.

jwnimmer-tri

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

third_party/spgrid/LICENSE line 1 at r1 (raw file):

I modified it to ...

Ideally, that edit would be part of the the second commit patch, so that the first commit is literally just upstream.

third_party/spgrid/LICENSE line 1 at r1 (raw file):

Previously, xuchenhan-tri wrote…

Working. I modified the commit history. Still need to find author information to fill in.

SGTM. You can use whatever you have, e.g.:

Co-Authored-by: "R. Setaluri, M. Aanjaneya, S. Bauer and E. Sifakis" <https://pages.cs.wisc.edu/~sifakis/project_pages/SPGrid.html>

The main goal is just to indicate that the commit is not an original work of authorship by you. Basically anything you write down would meet that goal.

xuchenhan-tri

+@jwnimmer-tri for feature review.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

third_party/spgrid/LICENSE line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

SGTM. You can use whatever you have, e.g.:
Co-Authored-by: "R. Setaluri, M. Aanjaneya, S. Bauer and E. Sifakis" <https://pages.cs.wisc.edu/~sifakis/project_pages/SPGrid.html>
The main goal is just to indicate that the commit is not an original work of authorship by you. Basically anything you write down would meet that goal.

Done

third_party/spgrid/LICENSE line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I modified it to ...

Ideally, that edit would be part of the the second commit patch, so that the first commit is literally just upstream.

Done

jwnimmer-tri

feature on the build system. I don't have the cycles to review the header + test, though.

Reviewed all commit messages.
Reviewable status: needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

xuchenhan-tri

Thanks Jeremy!

+@sherm1 for feature review on stuff under multibody/ if you are up for it.

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

sherm1

Will do.

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

xuchenhan-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/spgrid.h line 19 at r3 (raw file):

namespace internal {

// TODO(xuchenhan-tri): Add documentation on SpGrid.

Working.

I'm going to write something to make this PR more self-contained.

xuchenhan-tri

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/spgrid.h line 19 at r3 (raw file):

Previously, xuchenhan-tri wrote…

Working.

I'm going to write something to make this PR more self-contained.

Done. @sherm1 I added some documentation (and added a reference to the paper that describes the data structure). Hopefully it helps with the review

sherm1

The multibody/ stuff looks good with a few minor comments. Feature for that piece

Reviewed 7 of 20 files at r1, 3 of 3 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/spgrid.h line 105 at r4 (raw file):

  /* The maximum grid size along a single dimension. That is even
   though the grid is sparsely populated, the maximum grid size
   that can ever be allocated is kMaxGridSize^3. With 1cm grid dx, that

BTW with dx_ removed, the mention of dx here is less clear. Is that still a concept? Maybe define it here?

multibody/mpm/spgrid.h line 43 at r3 (raw file):

  Scalar dx() const { return dx_; }

  void Allocate(const std::vector<Offset>& offsets) {

BTW is GridData enumerable such that you could put function definitions in a cc file and instantiate them there?

multibody/mpm/spgrid.h line 82 at r3 (raw file):

   enough for most manipulation simulations. */
  static constexpr int kLog2MaxGridSize = 10;
  static constexpr int kMaxGridSize = 1 << kLog2MaxGridSize;

minor: I found the above confusing. The description sounds like there is a max grid size already and we're looking at one of its dimensions. But then I see this is actually setting kMaxGridSize to 1024. I also got confused by the "10 meters" followed by the "10"! The use of "size" here might be part of my confusion. This is really just a count of grid points so "size" in the sense of an array rather than a physical length as discussed in the previous sentence.

Consider starting this discussion by saying something like "Set the maximum number of grid points on a side to 1024."

xuchenhan-tri

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/spgrid.h line 43 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW is GridData enumerable such that you could put function definitions in a cc file and instantiate them there?

It's not really. I'll use one type of GridData heavily for most of the meat of MPM implementation, but might still use arbitrary grid data for unit tests.

multibody/mpm/spgrid.h line 82 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: I found the above confusing. The description sounds like there is a max grid size already and we're looking at one of its dimensions. But then I see this is actually setting kMaxGridSize to 1024. I also got confused by the "10 meters" followed by the "10"! The use of "size" here might be part of my confusion. This is really just a count of grid points so "size" in the sense of an array rather than a physical length as discussed in the previous sentence.

Consider starting this discussion by saying something like "Set the maximum number of grid points on a side to 1024."

I massaged the language a bit, PTAL.

multibody/mpm/spgrid.h line 105 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW with dx_ removed, the mention of dx here is less clear. Is that still a concept? Maybe define it here?

I avoid the term dx and use more descriptive language here instead.

sherm1

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

multibody/mpm/spgrid.h line 105 at r4 (raw file):

Previously, xuchenhan-tri wrote…

I avoid the term dx and use more descriptive language here instead.

Great, thanks!

xuchenhan-tri

+@ggould-tri for platform review (on Tuesday) per rotation please.

Reviewable status: LGTM missing from assignee ggould-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

ggould-tri

; I have not reviewed the third-party code in more than a superficial way.

Reviewed 15 of 20 files at r1, 3 of 3 files at r2, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 7 unresolved discussions, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @xuchenhan-tri)

a discussion (no related file):
BTW: The SpGrid class here seems useless -- it has no accessors, for instance -- but I assume that is because it is currently just a placeholder and real code will come later. I've noted some comments there but they might be wrong because of the incomplete nature of the class in which case you should dismiss and disregard them.

Unless you have a follow-up PR coming fairly immediately, you should put an explanatory comment in SpGrid summarizing what missing features will be added in the future; that will help to justify its design for later readers/authors.

multibody/mpm/test/spgrid_test.cc line 14 at r5 (raw file):

struct GridData {
  using Scalar = double;

nit: unused?

multibody/mpm/spgrid.h line 50 at r5 (raw file):

  - It must have a default constructor.
  - It must have a member function `set_zero()` that sets the data to zero.
  - The size of GridData must be a power of 2. */

BTW: Is this true? AFAICT from the code, it will be padded up to the next power of two size. Since it's difficult for users to know whether a given struct has a particular size (which is not guaranteed across compilers or architectures either), it would be better to relax this.

If on the other hand this is a true requirement, it would be better to statically assert it, e.g.

  constexpr bool is_power_of_two(size_t n) {
    return n && !(n & (n - 1));
  }
...
  static_assert(is_power_of_two(sizeof(GridData)));

multibody/mpm/spgrid.h line 66 at r5 (raw file):

  using Offset = uint64_t;

  SpGrid()

minor: I am somewhat confused by this ctor. It seems like the constructed object is useless -- none of its public methods can be called -- until Allocate is called. Instead of two-phase initialization, why not just replace the ctor with Allocate?

multibody/mpm/spgrid.h line 70 at r5 (raw file):

        blocks_(allocator_) {}

  void Allocate(const std::vector<Offset>& offsets) {

minor: The semantics of this public method are unclear; it needs a comment. I'm not sure I've correctly understood it, below.

Suggestion:

  /* Clears the contents of this SpGrid and ensures that a block is available
     at each offset in `offsets`. */
  void Allocate(const std::vector<Offset>& offsets) {

multibody/mpm/spgrid.h line 122 at r5 (raw file):

  // center of the objects so that the grid can be accommodated to the objects
  // that are translating.
  /* 3D coordinates in SPGrid starts at (i, j, k) = (0, 0, 0) with i, j, k

typo grammar

Suggestion:

  /* 3D coordinates in SPGrid start at (i, j, k) = (0, 0, 0) with i, j, k

multibody/mpm/spgrid.h line 132 at r5 (raw file):

  /* SPGrid allocator. */
  Allocator allocator_;
  /* Blocks containing grid nodes that are base nodes for at least one particle.

minor: This is the only use of the word "particle" in this code and its meaning is unclear.
Perhaps "occupied grid node" or something like that??

Code quote:

particle

xuchenhan-tri

Thanks! Comments addressed, PTAL.

Reviewable status: 3 unresolved discussions, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @xuchenhan-tri)

a discussion (no related file):