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

Add model name to the summary method #120

Closed
3 of 5 tasks
zaleslaw opened this issue Jun 15, 2021 · 8 comments · Fixed by #136
Closed
3 of 5 tasks

Add model name to the summary method #120

zaleslaw opened this issue Jun 15, 2021 · 8 comments · Fixed by #136
Assignees
Labels
good first issue Good for newcomers

Comments

@zaleslaw
Copy link
Collaborator

zaleslaw commented Jun 15, 2021

Our models could have names, but the current version of summary method (for both, Sequential and Functional models) doesn't support model name displaying.

The PR should include:

  • model name support for both, Functional and Sequential model
  • the missed model name case is handled correctly
  • SequentialCompilationTest.summary should be updated for validation model name
  • Test for the Functional model summary is added (you could use here model like ToyResNet)
  • (Optional) Refactoring of both summary methods (in Functional and Sequential models)
@zaleslaw zaleslaw added the good first issue Good for newcomers label Jun 15, 2021
@dosier
Copy link
Contributor

dosier commented Jun 15, 2021

I could do this one :)

@dosier
Copy link
Contributor

dosier commented Jun 15, 2021

@zaleslaw to what extend do both summary methods have to be refactored? E.g. does the method signature have to change at all?

@zaleslaw
Copy link
Collaborator Author

I've changed the ticket description regarding refactoring: the refactoring will be optional, the API is not under stability guarantees and could be changed.

The refactoring is just an idea, looks like it could be refactoring, but it's your choice as a contributor: propose the refactoring (with minor changes in the signature that doesn't affect all examples and tests) because there is some copy-paste there and common logic, but PR could be merged and without refactoring

@dosier
Copy link
Contributor

dosier commented Jun 16, 2021

@zaleslaw do the above commits cover the first 3 points in this issue sufficiently? Or is this not what you meant. And I can look into the refactoring yes.

@zaleslaw
Copy link
Collaborator Author

I've added the Contributing Guidelines. Please check this.

@zaleslaw
Copy link
Collaborator Author

You solution is simple and solves the problem

@dosier
Copy link
Contributor

dosier commented Jun 17, 2021

Maybe the refactor and writing a test is better suited as a part of #135 than this issue @zaleslaw ?

@zaleslaw
Copy link
Collaborator Author

I suggest do things step by step, the final form of model summary is unclear and there are a few moments related to logging, jupyter integration and so on.

I suggest to finish this ticket and if the form of summary will be changed, we will not ignore the modelname

dosier added a commit to dosier/KotlinDL that referenced this issue Jun 18, 2021
- based of toyresnet
- also offers base for other Functional model tests
@zaleslaw zaleslaw linked a pull request Jun 21, 2021 that will close this issue
zaleslaw pushed a commit that referenced this issue Jun 21, 2021
* Added missing saving functions for ReLU and ELU activation layers (JetBrains#78)

* Reverted changes to the imports

* Added "Model name: $name" line (if `name` is not null) to summary method #120

* Added model name validation in SequentialCompilationTest.summary #120

* Added "type" part to summary line for Sequential model #120

* Wrote simple test for Functional model summary (#120)
- based of toyresnet
- also offers base for other Functional model tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants