FluxML / MLJFlux.jl

Wrapping deep learning models from the package Flux.jl for use in the MLJ.jl toolbox
http://fluxml.ai/MLJFlux.jl/
MIT License
143 stars 17 forks source link

🚀 Instate documentation for MLJFlux #252

Closed EssamWisam closed 1 month ago

EssamWisam commented 2 months ago

Includes API index and placeholders for the documentation structure.

EssamWisam commented 2 months ago

@ablaom Know you prefer to review changes live (i.e., after deploying docs). Trying to recall whether that was possible before merging a PR with initial documentation and Github action.

EssamWisam commented 2 months ago

@ablaom this is now ready for a review. However, this includes only the pages/sections under the "Introduction" or "Interface" headlines in the sidebar (tutorials and workflow examples and contributing are still work in progress; even if I committed some work related to them).

ablaom commented 2 months ago

Perfect. I will likely review before the weekend.

P.S. This looked super polished in our offline discussion today. Thanks for the great work.

ablaom commented 2 months ago

Maybe is doesn't matter, but I don't ordinarily track docs/Manifest.toml.

ablaom commented 2 months ago

Still to do:

ablaom commented 2 months ago

While I will defer to your superior sense of aesthetics, I must admit a preference for serif fonts. I find them easier to read. I think screen resolutions are so fantastic these days that old arguments against Serif fonts don't apply.

But again, happy to defer to your judgement and taste.

EssamWisam commented 1 month ago

Thanks @EssamWisam . I've finished my initial review.

Thank you so much for your time and effort in this fantastic review, @ablaom. I think it would make it easier for me to automate the documentation deployment if we merge this (after finishing with the remaining comments and removing any pages yet to be added; e.g., like in workflow examples) and then after merging make another PR for tutorials/workflow examples.

The idea is that I will want the action to trigger the deploy "on PR to dev" and for it to take the effect docs must be deployable from dev. It's likely though that there is a way to do this without merging first but I'm not sure of it, just yet.

EssamWisam commented 1 month ago

While I will defer to your superior sense of aesthetics, I must admit a preference for serif fonts. I find them easier to read. I think screen resolutions are so fantastic these days that old arguments against Serif fonts don't apply.

But again, happy to defer to your judgement and taste.

If I remember correctly, I tried to copy the font used in Flux documentation just for uniformity. While I didn't even know about the screen resolution argument (which is now broken anyway); I do think that using serifs in a site makes go from feeling more modern and contemporary to more classical and formal. Maybe if we adopted a less fancy logo and put some equations in the homepage, I'd feel better with serif, but I'm not much drawn to it in this case. Likewise, I do think that people on average (esp. Gen Z kids like me) would think the same.

I know you didn't require this much justification but I also wanted to clarify why I think what I think.

ablaom commented 1 month ago

I know you didn't require this much justification but I also wanted to clarify why I think what I think.

Appreciate it! Yes, this may well be a generational thing, and I support your decision to keep the "modern" fonts.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.08%. Comparing base (b449d80) to head (ec6004f). Report is 18 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #252 +/- ## ======================================= Coverage 92.08% 92.08% ======================================= Files 12 12 Lines 316 316 ======================================= Hits 291 291 Misses 25 25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

EssamWisam commented 1 month ago

@ablaom I can't merge because I don't have write access. Could you do instead? I added the script for deployment but it may be the case that someone with access to the repo needed to go to settings/pages and ensure deploying is activated.

ablaom commented 1 month ago

@EssamWisam You should have an invite for access now.