microsoft / knossos-ksc

Compiler with automatic differentiation
Other
45 stars 10 forks source link

Use std::tuple instead of ks::Tuple in entry points #955

Closed dcrc2 closed 2 years ago

dcrc2 commented 3 years ago

This PR changes the entry points to use std::tuple, as this is supported directly by pybind11. This would mean that we could remove the specialization for ks::Tuple in knossos-pybind.h.

(Edited: The following comment was resolved by #961.)

One difficulty: we are optionally logging the entry point arguments using operator<<, but there is no operator<< defined for std::tuple. There are various ways we might fix this:

  1. Define operator<< for std::tuple. This invasion of std:: is technically illegal, but would probably work as long as no-one tries the same thing somewhere else.
  2. Define our own print function to use instead of operator<<.
  3. Log the values of the arguments after converting to ks types. This would work OK as things stand, but not once we are doing elementwise operations. (We wouldn't want to log inside the loop.)
  4. Just remove the logging now. As @toelli-msft pointed out on https://github.com/microsoft/knossos-ksc/pull/925#discussion_r669465277 it doesn't really make sense to have it turned on any more, unless you're debugging a small example.

Any thoughts? (@awf?)

toelli-msft commented 2 years ago

Can we be confident that this is fine from the point of view of performance? The reason for ks::Tuple existing at all is that it has better performance than std::tuple (under certain circumstances). Are there some tests or benchmarks we can put in place to make sure we don't degrade performance when switching to std::tuple here?

dcrc2 commented 2 years ago

Can we be confident that this is fine from the point of view of performance? The reason for ks::Tuple existing at all is that it has better performance than std::tuple (under certain circumstances). Are there some tests or benchmarks we can put in place to make sure we don't degrade performance when switching to std::tuple here?

I'm confident that any difference won't be measurable. The main reasons for this:

Since the goal of this PR is to remove the ks::Tuple-binding code, I don't think we can maintain a benchmark which compares the performance without defeating the purpose of the PR. I can try running the existing benchmarks before and after but it'll be surprising if there's any observable difference.

toelli-msft commented 2 years ago

Thanks for the explanation. I'm happy for you to proceed as you see fit.

awf commented 2 years ago

Happy to remove the logging, and happy with PR in general.