js-pytorch: Adding types and fixing bugs on TypeScript implementation
Changes necessary in the develop branch before merging into main:
- Adding types and fixing bugs of TypeScript implementation.
- Have zero errors pop up when running
npm run check. - Removing
tsconfig.jsonallowances before merging.
About this issue
- Original URL
- State: closed
- Created 3 months ago
- Reactions: 1
- Comments: 22 (22 by maintainers)
I don’t think I was being specific enough for which I apologise, no technically not errors of TS but using any essentially disables type checking and generally speaking we should try to use any very judiciously, there’s usually a way to narrow this type down. If absolutely necessary you can use an unknown type which is safer from a typing perspective, if we don’t know the type, it can’t be passed around or operated on without typescript throwing an error or being narrowed down further by a type guard.
https://tannerdolby.com/writing/writing-safe-code-with-typescript/ - see here. https://dmitripavlutin.com/typescript-unknown-vs-any/
Also if you haven’t check out the TS-ESLINT docs here https://typescript-eslint.io/rules/no-explicit-any/
Please check in case something with my local repo is wrong. Btw with Husky and lint-staged, this wont be a problem as it will not make the commits/push whilst there are linting/type check errors.
In the current updated code, i did a linting check (as the check script was removed in one of the commits)
They’re 99% about having any types (you can see this in ‘tensor’ class instance properties types in tensor.ts), there was around 230 before, so a good chunk has been done.
Quick work on TS errors! Think the jest syntax looks fine. Good catch on type checking in jest.
I think there’s definitely room for some unit tests in the future and maybe some refactoring of these three tests (not sure whether you’d call these e2e or integration) depending on future tests, extracting the neural net creation and some of the setup for example.
Mmm needs a bit of investigating, but the time taking in test running is mostly due to the last test, having played around with an initial jest setup, the first two tests take around 1-2 seconds which I think is acceptable, transformer test takes around 10seconds to complete. Do you happen to have the times prior to the PR for comparison (I’m using Macbook Pro m3 btw) ?
Couple of things that do stand out for me,
@eduardoleao052 i’ll work on utils.ts btw.