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

Doc adjustments #169

Merged
merged 11 commits into from
Nov 9, 2021
Merged

Doc adjustments #169

merged 11 commits into from
Nov 9, 2021

Conversation

jeremiahpslewis
Copy link
Contributor

No description provided.

@jeremiahpslewis
Copy link
Contributor Author

@gdalle Here’s my go through of the affine transform doc page. I’m new to the material so I may have introduced factual errors in pursuit of simplifying / adjusting. I defer to your judgement as to what to keep / what to skip. :)

@gdalle gdalle self-requested a review November 5, 2021 22:11
Copy link
Collaborator

@cscherrer cscherrer left a comment

Choose a reason for hiding this comment

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

Thanks @jeremiahpslewis ! I've made a few comments, and I'd also like @gdalle's thoughts on this before we merge

docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
Co-authored-by: Chad Scherrer <[email protected]>
docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
@jeremiahpslewis
Copy link
Contributor Author

@cscherrer Thanks for the review, I went ahead and added your suggestions and made a few further tweaks based on the direction of your feedback. Looking forward to hearing from @gdalle :)

docs/src/affine.md Outdated Show resolved Hide resolved
Co-authored-by: Moritz Schauer <[email protected]>
Copy link
Collaborator

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

I mainly spotted minor typos, but a more important comment would be the name Affine: why not AffineMeasure?

docs/src/affine.md Show resolved Hide resolved
docs/src/affine.md Outdated Show resolved Hide resolved
docs/src/affine.md Show resolved Hide resolved
docs/src/affine.md Show resolved Hide resolved
@cscherrer
Copy link
Collaborator

I mainly spotted minor typos, but a more important comment would be the name Affine: why not AffineMeasure?

We'll definitely have some things with Measure explicitly in the name (CountingMeasure), and others without (Normal). I'm not sure I've been entirely consistent on this, but generally my approach has been to prefer the shorter name unless it's confusing. We should talk more about this if you or others have other suggestions.

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #169 (cdca3dd) into master (7af6ad4) will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
- Coverage   45.31%   45.07%   -0.25%     
==========================================
  Files          32       32              
  Lines         673      670       -3     
==========================================
- Hits          305      302       -3     
  Misses        368      368              
Impacted Files Coverage Δ
src/realized.jl 51.56% <0.00%> (-2.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7af6ad4...cdca3dd. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2021

Package name latest stable
Mitosis.jl
Soss.jl

@gdalle
Copy link
Collaborator

gdalle commented Nov 9, 2021

Do we really have to issue a new release for such small, docs-only changes @cscherrer?

@cscherrer
Copy link
Collaborator

Not critical, but also not much reason not to have a minor release. They're free and pretty easy 🙂

@cscherrer
Copy link
Collaborator

cscherrer commented Nov 9, 2021

The one reason I see it could be nice is so people are more likely to see the new version of the docs

@cscherrer cscherrer merged commit 7102906 into JuliaMath:master Nov 9, 2021
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.

4 participants