Skip to content

Conversation

@mcabbott
Copy link
Member

@mcabbott mcabbott commented Oct 18, 2021

Might be nice to have?

Not entirely sure how well these will render in different places. Also whether this package tests its own jldoctests.

@DhairyaLGandhi
Copy link
Member

We could render them in the docs but large help sections that aren't critical can be annoying to use in the REPL.

@darsnack
Copy link
Member

I like this idea! All the activation function docstrings are much shorter than typical docstrings e.g. Conv. I think having a unicode plot in REPL output would be nice feature (it would still be shorter than most docstrings).

@mcabbott
Copy link
Member Author

I agree that huge docstrings you have to scroll through are a bit of a pain. I played with these a bit, and height=7 seems like a reasonable compromise size, at which you can roughly see what's going on, without taking the whole screen.

Screenshot 2021-10-18 at 20 26 06

@ToucheSir
Copy link
Member

How about under extended help? JuliaLang/julia#34226

@CarloLucibello
Copy link
Member

I think it is very helpful to have these plots under standard help. I also like the fact that they will show up in the documentation site.
Also, the docstrings' length shouldn't be a concern here, they are very short and you consult them very rarely

Project.toml Outdated
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
StableRNGs = "860ef19b-820b-49d6-a774-d7a799459cd3"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
UnicodePlots = "b8865327-cd53-5732-bb35-84acbb429228"
Copy link
Member

Choose a reason for hiding this comment

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

needs a compat bound

Copy link
Member Author

@mcabbott mcabbott Oct 19, 2021

Choose a reason for hiding this comment

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

I was following the other test deps (StableRNGs, Zygote) which don't have one.

Since there's no test dep on Documenter, I guess the docstrings aren't tested? Or maybe they are tested when building Flux's docs? Either way, there is then no need for a dependency here.

Copy link
Member Author

@mcabbott mcabbott Oct 19, 2021

Choose a reason for hiding this comment

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

Alternatively, we can add documenter just to test the doctests. That's probably not a bad idea, better to find out sooner if your PR needs to tweak something. The code needed is something like:

if VERSION < v"1.6-" || VERSION > v"1.7-"
    @warn "skipping doctests, on Julia $VERSION"  
else
    using Documenter
    @testset "doctests" begin
        doctest(NNlib, manual=false)
    end
end

But perhaps I declare that out-of-scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, got confused, doesn't need a compat entry (but i see you removed it entirely)

@DhairyaLGandhi
Copy link
Member

Maybe it's better to not have to depend on a plotting library even if light, considering that this isn't expected to be used often. Not to mention, NNlib is supposed to be light. We can still do it in the docs, it's applicability is higher in the docs anyway.

@mcabbott mcabbott merged commit 0995b15 into FluxML:master Oct 19, 2021
@mcabbott mcabbott deleted the unicodeplots branch October 19, 2021 21:28
@mcabbott
Copy link
Member Author

Build failures are (at least) two different isapprox tests of convolutions, plus Zygote on Julia 1.8

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.

5 participants