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

Fixing grammatical mistakes, typos, and adding description to the Viterbi algorithm #38

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

SatinWukerORIG
Copy link
Member

No description provided.

- On the other hand, write generic code where appropriate. Do not impose arbitrary limitations if the code can work on a wider range of data types.
- Don't use unsigned numerical types (`uint` and its sized variants), unless wrapping behaviour or binary manipulation is required for the algorithm.
- Don't use unsigned numerical types (`uint` and its sized variants), unless wrapping behavior or binary manipulation is required for the algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an international collaboration, so enforcing US spelling in docs is debatable (for symbols it's ok due to tradition).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Had I to choose, I would prefer UK spelling and grammar rules. US spelling/grammar feels omnipresent.
As a French speaker, I often mix US/UK usages. Usually, I use Grammarly to help me correct my syntax, but I did not use it for writing the CONTRIBUTING.md file.

@ZoomRmc
Copy link
Contributor

ZoomRmc commented Jun 22, 2023

Thanks a lot for the fixes, receiving a human read-through is invaluable!

Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

Very nice spelling improvement.
Please either rebase or create a new branch before each of your pull requests.
Your git log is a mess. It is making it hard to follow your changes.

@@ -29,7 +29,7 @@ An Algorithm is one or more functions that:
- return one or more outputs,
- have minimal side effects (Examples of side effects: `echo()`, `rand()`, `read()`, `write()`).

## Contributor agreement
## Contributor Agreement
Copy link
Collaborator

@dlesnoff dlesnoff Jun 22, 2023

Choose a reason for hiding this comment

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

We should have a consistent rule for titles.

I propose lowercasing all but the first letter. That is the rule that I use in my research papers. Revert this change and lowercase all the other titles.

Comment on lines 3 to +4
The Catalan numbers are a sequence of natural numbers that occur in the
most large set of combinatorial problems.
the largest set of combinatorial problems.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can never know in advance how many combinatorial sets of problems we will discover. Maybe we will find one more general than Catalan numbers in the future. Just let's rephrase this to say that it is very frequent.

Comment on lines +7 to +8
## Time Complexity: O(K2n)
## Space Complexity: O(Kn)
Copy link
Collaborator

@dlesnoff dlesnoff Jun 22, 2023

Choose a reason for hiding this comment

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

What is K and what is n?
Why one is lowercase and not the other?
Drop the constant in the time complexity. If you insist on the constant, put it in front.

##
## Time Complexity: O(K2n)
## Space Complexity: O(Kn)
## https://www.cs.cmu.edu/~epxing/Class/10708-20/scribe/lec4_scribe.pdf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move the Wikipedia link under this link.

@@ -1,4 +1,14 @@
# Viterbi
## The Viterbi algorithm is a dynamic programming algorithm
## for obtaining the maximum a posteriori probability estimate
## of the most likely sequence of hidden states
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## of the most likely sequence of hidden states
## of the most likely sequence of hidden states in a hidden Markov model.

@Panquesito7 Panquesito7 added the documentation Improvements or additions to documentation label Jun 23, 2023
SatinWukerORIG and others added 12 commits June 27, 2023 11:44
Fixing grammatical mistakes
Adding description and reference to the Viterbi Algorithm
* Add a sample of maths basic algorithms

* Update maths/abs.nim

Co-authored-by: Zoom <[email protected]>

* Use openArray in absMaxSort

Co-authored-by: Zoom <[email protected]>

* Fix seq->openArray and int->SomeInteger

* Use Positive as input instead

* Update maths/addition_without_arithmetic.nim

Co-authored-by: Pietro Peterlongo <[email protected]>

* Name allocation number

* [maths/abs] Replace maxAbsSort by signed[Min/Max]Abs

* [Aliquot Sum] Add header

* Add empty line at end of file

* Remove Allocation number since it is a non standard algorithm

* Fix titles

* Run nimpretty on the files

* Rename file and add DIRECTORY.md

* abs: Fix variable name

* Add export marker

Co-authored-by: Zoom <[email protected]>

* Add RE block in aliquot sum and improve RE

Co-authored-by: Zoom <[email protected]>

* Remove MD quotation marks for inline code

* Add export marker for RE block

* Remove extra ValueError

---------

Co-authored-by: Dimitri LESNOFF <[email protected]>
Co-authored-by: Zoom <[email protected]>
Co-authored-by: Pietro Peterlongo <[email protected]>
Co-authored-by: David Leal <[email protected]>
* Use a dynamic allocated array (sequence) for strings

* Run nimpretty

* Add requested changes

* Fix documentation generation

* Fix test title according to changes

Co-authored-by: Satin Wuker <[email protected]>

* Update comments to warn about indexing issues

Modified a bit the suggestions of comments. Please check them before approving.

Co-authored-by: Zoom <[email protected]>

---------

Co-authored-by: Dimitri LESNOFF <[email protected]>
Co-authored-by: Satin Wuker <[email protected]>
Co-authored-by: Zoom <[email protected]>
…hms#41)

This will run `sudo apt-get update` before running any other commands on the Gitpod prebuild image (`.gitpod.dockerfile`).
…s#42)

* Update README.md

Use more extended markdown syntax instead of HTML,
Add Matrix links
Superseed PR TheAlgorithms#40

* Actually fix gitter! Move img src links to footer.

Also changed the TheAlgorithms logo image link.

* Restore TheAlgorithms logo link

* More link fixes

---------

Co-authored-by: dlesnoff <[email protected]>
@dlesnoff
Copy link
Collaborator

dlesnoff commented Jun 27, 2023

Please, look at the edit below.

I made a huge error on your branch. I tried to rebase and force push to remove these commits about linear search and bubble sort. By doing so, I put new commits that were not on your branch on top of yours, thus modifying the commit timestamps.
If your local repo is still fine, just git push --force.
I wanted to rebase all these old commits you had.
You will have to remove these by rebasing.
Just use after the forced push and the rebasing with main, you should rebase your local repo: git rebase -i origin/main and delete all these commits for linear search and bubble sort.
This will make the commit history cleaner (and the commit description shorter).
The problem is, that you will have to put your commits on top of those on the main branch and git push --force at some point.

EDIT: I think that merging with main solved the issue.

@dlesnoff dlesnoff mentioned this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants