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

Clean up #17

Open
3 of 6 tasks
dan-zheng opened this issue Oct 5, 2018 · 1 comment
Open
3 of 6 tasks

Clean up #17

dan-zheng opened this issue Oct 5, 2018 · 1 comment
Labels
cleanup Technical debt that need to be paid off good first issue Good for newcomers help wanted Extra attention is needed

Comments

@dan-zheng
Copy link
Collaborator

dan-zheng commented Oct 5, 2018

A big list of clean up opportunities, vaguely ordered by importance.
Feel free to add/edit.

  • Revamp tensor initializers, especially ones that take both "data" and "shape" arguments.
    • "Data" arguments should be clearly distinguished from "shape" arguments.
    • Tensor.ones(2, 3) is fine. Tensor.fill(1, 2, 3) is not.
    • Use multiple parameter lists for disambiguation: Tensor.fill(2, 3) { is => idx += 1; idx }
  • Done. Use either Manifest or Typ consistently. Typ is not native for our version of LMS.
  • Improve testing.
    • Currently, all tests roughly look like this:
        test("foobar") {
          val test1 = new DslDriverC[String, Unit] with TensorExp {
            def snippet(a: Rep[String]): Rep[Unit] = {
              // `a` is a dummy that's not actually used.
              Tensor.assertEqual(...)
            }
          }
          test1.eval("a")
        }
      This is not ideal because the Rep[String] argument is never used. Can we make this nicer (does DslDriverC[Unit, Unit] work)?
    • Done. Create a helper test utility class (e.g. LanternTestSuite) that extends org.scalatest.FunSuite. This class can define helpers like def runTest(snippet: DslDriverC[String, Unit]).
  • Done. Rename files.
    • The filenames ad_lms.scala and ad_lms_vector.scala aren't very clear to me. Perhaps ScalarDifferentiation.scala and TensorDifferentiation.scala are more clear.
    • Pascal case all filenames.

For the most part, these items are low priority. We can pursue them lazily.

@dan-zheng dan-zheng added cleanup Technical debt that need to be paid off help wanted Extra attention is needed good first issue Good for newcomers labels Oct 5, 2018
@GSAir
Copy link
Collaborator

GSAir commented Oct 9, 2018

I am not sure how to add an item to the list:

  • be consistent with usage Manifest or Typ. Typ is not native for the version of LMS we are using.

GSAir added a commit that referenced this issue Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Technical debt that need to be paid off good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants