-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Making the output of model.summary() easier to read. #11795
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
Conversation
Long term update of local repo
|
I am not sure if it's just me, but I think this is better and could just replace the previous one? |
|
Seems clear enough with more complex layer inputs/outputs too... |
|
I don't like the |
|
I updated the commit to just remove the underlines between layers. No short= option now. The people have spoken. |
|
Need @fchollet review since UX related. |
|
I like the new style :) |
|
I like it too. |
|
I'm not sure how to get it to pass the Travis build test(s). I fixed an
indentation issue, but twice it's failed with a timeout, or something
else I'm not seeing.
…On Sun, Dec 9, 2018 at 9:01 PM Fariz Rahman ***@***.***> wrote:
I like it too.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#11795 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ATYK_fvNybhXOeAtYjOCdPU_27OhORFEks5u3eqLgaJpZM4ZBe5B>
.
|
|
I don't think it's related to your PR. Please wait for #11521 to be merged into master. This should fix the timeout. |
|
Let's go with the new style, but I think we can improve it further. I think we need column separators, especially useful when columns are overlapping. Let's try to add that? |
|
If we add delimiters, it will look like this: It can easily become markdown compatible: Which becomes:
The text version isn't as nice, but we get markdown for free. What do you think? |
|
I'm worried about what happens with multi-input layers, where there are multiple lines needed for the rightmost column. Could you show an example? I think 99.9% of usage will be in terminals or notebooks, so we should optimize for that use case only. People who want markdown can convert the previous output to markdown in just a couple of text editor commands. |
|
Indeed, compatibility is going to make a lot of things harder. The gain is small but not enough to justify the complexity increase. |
In any case, here is a sample with multiple inputs, and a sub-model (a convnet) used twice. It seems to produce a nice markdown table, and the text isn't ugly, but it does have the '|''s prepended.
|
|
Here it is without the prefixed pipes, and with underscores to preserve otherwise blank fields:
Total params: 19,305,471 |
|
The version from here is the best so far, so let's go with that #11795 (comment) Is there anything we can do to make it easier to read the last column when there are layers that have multiple inputs? |
Maybe adding blank rows only around layers with multiple inputs could work. It's a bit more readable in my opinion. @fchollet @gabrieldemarmiesse @jaggzh However, I am not sure of the complexity that such an implementation might induce. |
|
I propose another way of handling layers with multiple inputs: quite pytonic and close to how the layer is called on the tensors. Opinions? |
|
@gabrieldemarmiesse that looks neat. Also, what about changing "Connected to" to "Inputs"? |
|
@farizrahman4u yes that makes way more sense since we are displaying tensors, not layers. |
+1 The use of In terms of visual style, I think this #11795 (comment) is the best looking one. If we make sure we cut bleeding text (like |
|
@MrinalJain17 The blank lines don't look as bad as one might expect. Thanks for that. I think, though, that we can balance minimal-size, visual appeal, and consistency, though. @fchollet My personal tendency is still to provide options to users in some way. They might load up some arbitrary model and print the summary(), so a nice default, but then the user can set an option: In any case. I made a sample default below. Notes:
...what do you think about this for the current version (and default if we offer options in the future, or separate calls, like model.summary_csv()). But for now: |
I would vote against adding options for those reasons:
We have a line_lenght argument. See https://keras.io/utils/#print_summary . If the text fits, it might be preferable to put everything in the same line. Otherwise, we can use your solution and use a blank line. +1 for the separators (vertical and horizontal) they look nice. |
|
The new example looks great, thanks a lot. We can go with this! I agree with @gabrieldemarmiesse that we shouldn't add more formatting options at this time. |
…ng units This shows a trainable parameter in the full data frame and trainable vs non-trainable params in the summary. This should be like keras [example](keras-team/keras#11795 (comment)). I also changed the pandas display so that instead of putting (M) or (K) manually in the label, pandas appends it to the number automatically.
So should this PR be closed then? |
|
I unfortunately got too involved with other issues in life and haven't had
the time/priorities for this particular task, and likely won't any time
soon. I presume, when I'm working on another Keras model again, the output
will bother me and I might return to it at that time... Not sure if I'll
have the access to re-open the issue myself though. :)
…On Fri, Aug 23, 2019 at 9:55 AM Philip May ***@***.***> wrote:
The new example looks great, thanks a lot. We can go with this! I agree
with @gabrieldemarmiesse <https://github.com/gabrieldemarmiesse> that we
shouldn't add more formatting options at this time.
So should this PR be closed then?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11795?email_source=notifications&email_token=AE3AV7P3VWMWOP7V5LXZZE3QGAI7NA5CNFSM4GIF5ZA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5AYENA#issuecomment-524386868>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE3AV7O4I3GA3QBY2ISUAZDQGAI7NANCNFSM4GIF5ZAQ>
.
|
|
Thank you for preparing this PR. We are no longer adding new features to multi-backend Keras, as we are refocusing development efforts on I think improving the UX of |
Summary
Added an option to model.summary() for a bit more brief output.
Adding short=True will now just disable the underlines between layers.
The output might be unclear on complex models, but it's up to the caller.
Example outputs with normal (default) summary, and with short=True:
Default model.summary() (eg. short=False):
With model.summary(short=True):
Related Issues
PR Overview