Skip to content

Conversation

@ChengYen-Tang
Copy link
Contributor

/// <exception cref="ArgumentException"></exception>
/// <exception cref="InvalidOperationException"></exception>
public virtual void register_module(string name, Module submodule)
public virtual void add_module(string name, Module module)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they are aliases, I would suggest leaving register_module() in place and virtual, and then define add_module() as non-virtual and have it call register_module(). There's no reason for both of them to be virtual, and in case anyone has already overriden register_module, you would have a breaking code change if it's no longer virtual.

@NiklasGustafsson
Copy link
Contributor

Hi @ChengYen-Tang -- I usually ask all contributors to introduce themselves in this thread:

#333

It is voluntary, but it's nice for everyone to know who's helping with TorchSharp.

@NiklasGustafsson NiklasGustafsson linked an issue Feb 6, 2023 that may be closed by this pull request
…odule() as non-virtual and have it call register_module().
@ChengYen-Tang
Copy link
Contributor Author

@NiklasGustafsson,

Is this okay.

@NiklasGustafsson
Copy link
Contributor

@NiklasGustafsson,

Is this okay.

It looks okay to me. As soon as I can get a clean test run (they intermittently fail due to concurrency), I'll merge.

@ChengYen-Tang
Copy link
Contributor Author

ChengYen-Tang commented Feb 7, 2023

@NiklasGustafsson,
Is this okay.

It looks okay to me. As soon as I can get a clean test run (they intermittently fail due to concurrency), I'll merge.

Can't this problem be solved?
I can't figure out why the process crashes

@NiklasGustafsson NiklasGustafsson merged commit 1a5b463 into dotnet:main Feb 7, 2023
@ChengYen-Tang ChengYen-Tang deleted the 904 branch March 9, 2023 05:28
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.

Missing add_module api

2 participants