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

Big docs & readme update #98

Merged
merged 19 commits into from
Aug 14, 2021
Merged

Big docs & readme update #98

merged 19 commits into from
Aug 14, 2021

Conversation

barucden
Copy link
Collaborator

Okay, this one's a little bigger.

The goal is to simplify the docs: to contain only what's this package about so we can keep it up-to-date, no duplicates (rather use cross-references), simpler structure, less talking.

The biggest change in README is the addition of contributing section.

Main changes in docs:

  • Menu structure is simpler
  • New example for integration with Flux.jl
  • Added docs for MapFun and AggregateThenMapFun
  • Removed the "Working with images in Julia" page (instead, it is only a tiny section in introduction that refers to JuliaImages docs)
  • Removed indices.md

I plan to do a few more smaller changes in the future (like adding more examples, simplify further, etc.)

Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

I didn't check every single line but it looks good to me, I also agree some of the contents here are redundant. I kept some of the old contents during the transition because I didn't think of a good replacement for them. As a non-native English speaker, I enjoy @Evizero's words a lot.

docs/src/index.md Show resolved Hide resolved
docs/Project.toml Outdated Show resolved Hide resolved
@barucden
Copy link
Collaborator Author

barucden commented Aug 12, 2021

Yeah, the sentences read nicely. For example, the part about images in Julia was quite educational. I almost felt pity removing it. On the other hand, maintaining the documentation is perhaps harder than maintaining the code, so I aimed to cut down anything that is not necessary (in this specific case, the documentation is available elsewhere -- JuliaImages docs). Also, I think having less text is better for the users as it's easier to skim through.

Edit: Let's keep this PR open for a while so @Evizero has a chance to express his opinion.

@Evizero
Copy link
Owner

Evizero commented Aug 13, 2021

Thanks for the kind words. While it was fun writing the images tutorial page, I do think it became outdated quite a while ago. I defer to your combined judgement which has been excellent so far. Thank you for your hard work

Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Very well written! The "Label-preserving Transformations" part is very educational; I believe most people are just ignorant about this.

I reckon that we should emphasize the importance of Colorant array. Many issues in using this package are due to the old habit of the plain Array{Float64, 3} data. I didn't comment on the "Complete example" section so you might need to update it as well.

README.md Outdated Show resolved Hide resolved
docs/operations/misc/general.jl Outdated Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Outdated Show resolved Hide resolved
docs/src/examples/flux.md Show resolved Hide resolved
Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

@barucden Unless requested, I don't have more comments in. With some final word tweaks, I think we're good to go. It's up to you.

@barucden
Copy link
Collaborator Author

Thanks for the feedback! I am merging it in now. We can continue to improve the documentation iteratively.

@barucden barucden merged commit 8d80713 into Evizero:master Aug 14, 2021
@barucden barucden deleted the readme-update branch August 14, 2021 11:49
Comment on lines +1 to +4
# ---
# title: Higher-order functions
# description: a set of helper opeartions that allow applying any function
# ---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we need a cover image here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it required? I could not think of any since the operations are really general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, but I guess it somehow doesn't look very nice if using the fallback cover... I really have a bad taste in designing the icon/cover. :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try to add a better cover picture in near future

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.

3 participants