Implementations for methods for machine_views and associated modules

[Marsella8]

Description of changes:

Implemented NOT_IMPLEMENTED() functions within machine_view.cc, strided_rectangle.cc, strided_rectangle_sides.cc, device_id.cc.

Related Issues:

Linked Issues:

• Issue #

Issues closed by this PR:

• Closes #

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.05%. Comparing base (d30c8f0) to head (61ae1c6).
Report is 6 commits behind head on repo-refactor.

Additional details and impacted files

@@                Coverage Diff                @@
##           repo-refactor    #1429      +/-   ##
=================================================
+ Coverage          38.78%   40.05%   +1.26%     
=================================================
  Files                256      258       +2     
  Lines               8973     9096     +123     
  Branches             331      334       +3     
=================================================
+ Hits                3480     3643     +163     
+ Misses              5493     5453      -40     

Flag	Coverage Δ
unittests	40.05% <100.00%> (+1.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
lib/pcg/src/pcg/machine_view.cc	92.85% <100.00%> (+92.85%)	⬆️
lib/pcg/src/pcg/strided_rectangle_side.cc	100.00% <100.00%> (+100.00%)	⬆️
lib/pcg/src/strided_rectangle.cc	100.00% <100.00%> (+100.00%)	⬆️
lib/pcg/test/src/test_machine_view.cc	100.00% <100.00%> (ø)
lib/pcg/test/src/test_strided_rectangle.cc	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[lockshaw]

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Marsella8)

---

lib/pcg/src/strided_rectangle.cc line 24 at r2 (raw file):

num_points_t get_num_points(StridedRectangle const &rect) {
  int num = 1;
  for (StridedRectangleSide const &side : rect.sides) {

Replace with product from utils/containers.h?

---

lib/pcg/src/pcg/machine_view.cc line 79 at r2 (raw file):

  }
  assert(get_device_type(start) == DeviceType::GPU);
  return make_1d_machine_view(unwrap_gpu(start), num_points, stride);

Suggestion:

  } else {
    assert(get_device_type(start) == DeviceType::GPU);
    return make_1d_machine_view(unwrap_gpu(start), num_points, stride);
  }

---

lib/pcg/src/pcg/machine_view.cc line 108 at r2 (raw file):

  }
  assert(get_device_type(start) == DeviceType::GPU);
  return make_1d_machine_view(unwrap_gpu(start), interval_size, stride);

Suggestion:

  } else {
    assert(get_device_type(start) == DeviceType::GPU);
    return make_1d_machine_view(unwrap_gpu(start), interval_size, stride);
}

---

lib/pcg/test/src/test_machine_view.cc line 22 at r2 (raw file):

    device_id_t start_cpu{cpu_id_t{3}};

    MachineView gpu_mv{start_gpu, rect};

Split gpu and cpu processing into subcases--the "declare a whole bunch of stuff, then run a whole bunch of tests on it" makes testcases rather hard to read. It's a lot more readable if the declarations are closer to and associated with their uses

---

lib/pcg/test/src/test_machine_view.cc line 25 at r2 (raw file):

    MachineView cpu_mv{start_cpu, rect};

    CHECK(make_1d_machine_view(

Can you wrap these in subcases with names that clarify the differences between all these different checks? (if it's testing overloads, for example, it could be SUBCASE("<the function signature>"))

---

lib/pcg/test/src/test_strided_rectangle.cc line 12 at r2 (raw file):

    StridedRectangleSide result =
        strided_side_from_size_and_stride(side_size_t{7 * 5}, 5);
    CHECK(result == side);

Separate test case (or at least a separate subcase)

Code quote:

    StridedRectangleSide result =
        strided_side_from_size_and_stride(side_size_t{7 * 5}, 5);
    CHECK(result == side);

---

lib/pcg/test/src/test_strided_rectangle.cc line 24 at r2 (raw file):

    CHECK(get_num_dims(rect) == 3);
    CHECK(get_side_at_idx(rect, ff_dim_t{0}) == s0);

At the very least subcases for different functions

[Marsella8]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @lockshaw)

---

lib/pcg/src/strided_rectangle.cc line 24 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Replace with product from utils/containers.h?

Changed (rect.sides is of type FFOrdered so direct application of transform is not possible, let me know if this is preferrable).

---

lib/pcg/src/pcg/machine_view.cc line 79 at r2 (raw file):

  }
  assert(get_device_type(start) == DeviceType::GPU);
  return make_1d_machine_view(unwrap_gpu(start), num_points, stride);

Done.

---

lib/pcg/src/pcg/machine_view.cc line 108 at r2 (raw file):

  }
  assert(get_device_type(start) == DeviceType::GPU);
  return make_1d_machine_view(unwrap_gpu(start), interval_size, stride);

Done.

---

lib/pcg/test/src/test_machine_view.cc line 22 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Split gpu and cpu processing into subcases--the "declare a whole bunch of stuff, then run a whole bunch of tests on it" makes testcases rather hard to read. It's a lot more readable if the declarations are closer to and associated with their uses

Done.

---

lib/pcg/test/src/test_machine_view.cc line 25 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Can you wrap these in subcases with names that clarify the differences between all these different checks? (if it's testing overloads, for example, it could be SUBCASE("<the function signature>"))

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 12 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Separate test case (or at least a separate subcase)

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 24 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

At the very least subcases for different functions

Done.

[lockshaw]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Marsella8)

---

lib/pcg/src/strided_rectangle.cc line 24 at r2 (raw file):

Previously, Marsella8 wrote…

Changed (rect.sides is of type FFOrdered so direct application of transform is not possible, let me know if this is preferrable).

It's defined here: https://github.com/flexflow/FlexFlow/blob/repo-refactor/lib/op-attrs/include/op-attrs/dim_ordered/transform.h

---

lib/pcg/test/src/test_machine_view.cc line 12 at r3 (raw file):

    gpu_id_t start(1);
    MachineView mv{device_id_t{start}, rect};
    CHECK(num_dims(mv) == 2);

Subcases?

---

lib/pcg/test/src/test_machine_view.cc line 24 at r3 (raw file):

    SUBCASE("make_1d_machine_view(gpu_id_t start, gpu_id_t stop, int stride)") {
      CHECK(make_1d_machine_view(
                start_gpu, device_id_t{gpu_id_t(1 + 7 * 5)}, 5) == gpu_mv);

Follow this pattern where possible for clarity, apply to the subcases below

Suggestion:

      MachineView result = make_1d_machine_view(start_gpu, device_id_t{gpu_id_t(1 + 7 * 5)}, 5);
      MachineView correct = gpu_mv;
      CHECK(result == correct);

---

lib/pcg/test/src/test_strided_rectangle.cc line 6 at r3 (raw file):

TEST_SUITE(FF_TEST_SUITE) {
  TEST_CASE("get_side_size(StridedRectangleSide s)") {

Suggestion:

  TEST_CASE("get_side_size(StridedRectangleSide)") {

---

lib/pcg/test/src/test_strided_rectangle.cc line 11 at r3 (raw file):

    CHECK(get_side_size(side) == side_size_t{7 * 5});
  }
  TEST_CASE("Make StridedRectangleSide from size and stride") {

If you're testing a function, just put the function name

Suggestion:

  TEST_CASE("strided_side_from_size_and_stride") {

---

lib/pcg/test/src/test_strided_rectangle.cc line 12 at r3 (raw file):

  }
  TEST_CASE("Make StridedRectangleSide from size and stride") {
    StridedRectangleSide side{num_points_t{10}, 3};

Suggestion:

    StridedRectangleSide correct{num_points_t{10}, 3};

---

lib/pcg/test/src/test_strided_rectangle.cc line 25 at r3 (raw file):

    StridedRectangle rect{{s0, s1, s2}};

    SUBCASE("number of dimensions") {

Suggestion:

    SUBCASE("get_num_dims") {

---

lib/pcg/test/src/test_strided_rectangle.cc line 28 at r3 (raw file):

      CHECK(get_num_dims(rect) == 3);
    }
    SUBCASE("number of points") {

Suggestion:

   SUBCASE("get_num_points") {

---

lib/pcg/test/src/test_strided_rectangle.cc line 31 at r3 (raw file):

      CHECK(get_num_points(rect) == num_points_t{7 * 8 * 10});
    }
    SUBCASE("each side is correctly stored") {

Suggestion:

  SUBCASE("get_side_at_idx")

[Marsella8]

Reviewable status: 5 of 8 files reviewed, 9 unresolved discussions (waiting on @lockshaw)

---

lib/pcg/src/strided_rectangle.cc line 24 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

It's defined here: https://github.com/flexflow/FlexFlow/blob/repo-refactor/lib/op-attrs/include/op-attrs/dim_ordered/transform.h

Fixed.

---

lib/pcg/test/src/test_machine_view.cc line 12 at r3 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Subcases?

Done

---

lib/pcg/test/src/test_machine_view.cc line 24 at r3 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Follow this pattern where possible for clarity, apply to the subcases below

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 6 at r3 (raw file):

TEST_SUITE(FF_TEST_SUITE) {
  TEST_CASE("get_side_size(StridedRectangleSide s)") {

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 11 at r3 (raw file):

Previously, lockshaw (Colin Unger) wrote…

If you're testing a function, just put the function name

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 12 at r3 (raw file):

  }
  TEST_CASE("Make StridedRectangleSide from size and stride") {
    StridedRectangleSide side{num_points_t{10}, 3};

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 25 at r3 (raw file):

    StridedRectangle rect{{s0, s1, s2}};

    SUBCASE("number of dimensions") {

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 28 at r3 (raw file):

      CHECK(get_num_dims(rect) == 3);
    }
    SUBCASE("number of points") {

Done.

---

lib/pcg/test/src/test_strided_rectangle.cc line 31 at r3 (raw file):

      CHECK(get_num_points(rect) == num_points_t{7 * 8 * 10});
    }
    SUBCASE("each side is correctly stored") {

Done.

[lockshaw]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marsella8)