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

Reviewed all modified modules and continued, made adjustments and continued with rehaul #16

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

shaltielshmid
Copy link

@shaltielshmid shaltielshmid commented Mar 10, 2024

Hi @NiklasGustafsson !
I went over all the modified files in the PR in the dotnet repo. Mostly they were great - I made a slight adjustment here and there (like the wrong class name in nameof or a python naming change).

All the modules that you moved to ParamLess but were still just a wrapper for the unmanaged object - I removed the unmanaged dependency. This took quite a lot of effort for the all the different pooling functions.

In general, for pooling, the functions that return with_indices, I changed all the tuples to be named (Tensor Values, Tensor Indices) to match other usages of that kind of tuple.

I had two debates that I didn't put in yet for Linear and Bilinear:
1] The constructor has an argument hasBias whereas in PyTorch it is just bias. Is that worth fixing?
2] The properties in_features etc. are public properties which can be modified - the question is whether we want to set them to be private set? I wasn't sure since the weight field can be modified.

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