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

ARModel: make interface use FluidTensorView #1

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

weefuzzy
Copy link

@weefuzzy weefuzzy commented Mar 2, 2022

Hello,

As discussed, updating interface of ARModel to use FluidTensorView rather than raw pointers, so that the sizes and offsets of the views could make it slightly easier to express / follow intentions w/r/t reading before pointers etc.

Bits of this you may find aesthetically objectionable. I've buggered about more with the little family of [model | forward | backward] [Error | Predict]() methods than originally imagined.

  • I decided that I'd rather be able to assert in one place, so I've changed things so that the scalar functions are special cases of the array functions rather than vice versa,
  • now comes down to specializations of modelPredict, which gets passed a different function object depending on whether it's doing a prediction or reporting an error. It probably warrants renaming.
  • I've changed the ordering of params to inputs-then-outptus, because this is what we use everywhere else.

Personally, I'm happy that this is improvement in terms of readability and reason-about-ness, but then I'm (obviously) very comfortable with reading code with FluidTensorView manipulations in it. I do like that we need to be explicit about offsets and sizes at call sites, and that it's possible to drop assertions in pretty much throughout the call stack to ensure that assumptions about data before and after specific pointer locations are true.

After some false starts, it also passes the same (pretty lightweight) tests as the previous commit. (C++ equivalent tests still need updating...)

@AlexHarker AlexHarker changed the base branch from armodel-robust-iteration-bug to armodel-review March 3, 2022 12:40
@AlexHarker AlexHarker merged commit 9970a92 into AlexHarker:armodel-review Mar 3, 2022
@weefuzzy weefuzzy deleted the armodel/interface branch March 3, 2022 12:41
@weefuzzy weefuzzy restored the armodel/interface branch March 3, 2022 12:43
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.

2 participants