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

Statistic Module added #243

Closed
wants to merge 15 commits into from
Closed

Conversation

kpkalwa
Copy link

@kpkalwa kpkalwa commented Apr 15, 2021

No description provided.

@jgonggrijp jgonggrijp added after modules This should be postponed until after modularization (temporary label, see #220) enhancement labels Apr 15, 2021
Copy link
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Thanks for carrying over your work from jashkenas/underscore#2920, @kpkalwa! We'll continue the review process here.

Let's start with ensuring that the copy is 100% faithful to your Underscore branch. I currently see one missing module: you didn't copy modules/sum.js yet.

modules/index.collections.statistics.js Outdated Show resolved Hide resolved
modules/percentile.js Outdated Show resolved Hide resolved
modules/standardError.js Outdated Show resolved Hide resolved
modules/variance.js Outdated Show resolved Hide resolved
test/collections.statistics.js Outdated Show resolved Hide resolved
test/collections.statistics.js Outdated Show resolved Hide resolved
test/collections.statistics.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Alright, seems like you copied all your code over and even fixed some indentation that I missed before! Good work.

The next step is to remove the pre-existing modules that you copied from Underscore (_baseIteratee.js, extendOwn.js, etcetera, not your own additions) and to update the import paths in your own new modules so that original Underscore modules are imported from the underscore package instead of from the same directory. This will truly integrate your code in Underscore-contrib, which is a library designed to depend on Underscore as an external dependency.

There will be two more steps after this: (1) add documentation and (2) clean up version history so that the original Underscore modules don't take up any space in the repository. Let's take this one step at a time, though.

modules/mean.js Outdated Show resolved Hide resolved
modules/mean.js Show resolved Hide resolved
modules/median.js Outdated Show resolved Hide resolved
modules/mode.js Outdated Show resolved Hide resolved
modules/percentile.js Outdated Show resolved Hide resolved
modules/statRange.js Outdated Show resolved Hide resolved
modules/sum.js Outdated Show resolved Hide resolved
modules/sum.js Outdated Show resolved Hide resolved
modules/variance.js Outdated Show resolved Hide resolved
if (typeof iteratee == 'number' && collection != null && typeof collection[0] != 'object') {
iteratee = null;
}
iteratee = cb(iteratee, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
iteratee = cb(iteratee, context);
iteratee = _.iteratee(iteratee, context);

@jgonggrijp
Copy link
Contributor

Keep it up @kpkalwa, you're doing well!

You didn't follow up on the last change request in my most recent review. Please add another commit to complete it.

Next move is adding documentation. You can create a new docs/underscore.collections.statistics.md, following the same structure as in (for example) docs/underscore.array.builders.md. It's OK if it's a bit brief/rough for now.

@kpkalwa
Copy link
Author

kpkalwa commented May 11, 2021

Can you review the changes

@jgonggrijp
Copy link
Contributor

@kpkalwa I did, see here and here. You still have to follow up on those comments.

@kpkalwa
Copy link
Author

kpkalwa commented Oct 21, 2021

Document for the statistics added @jgonggrijp

Copy link
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Thank you for resuming this work, @kpkalwa!

I see at least one loose end, but this is definitely moving in the right direction. The documentation looks promising.

Comment on lines 32 to 34
If the array has odd numbers then median is the middle element.

If the array has even numbers then average of middle two numbers is the median value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd/even number of numbers. For brevity and less confusion, though, I suggest "length".

Suggested change
If the array has odd numbers then median is the middle element.
If the array has even numbers then average of middle two numbers is the median value.
If the array has odd length then median is the middle element.
If the array has even length then average of middle two numbers is the median value.


#### mean

Signature: `_.mean(... arrays:Array ...)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really arrays, plural?


Signature: `_.sum(... arrays:Array ...)`

The `_.sum` function calculates the sum of the given arrays.
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume it sums a single array.

Suggested change
The `_.sum` function calculates the sum of the given arrays.
The `_.sum` function calculates the sum of the given array.


Signature: `_.variance(... arrays:Array ...)`

The `_.variance` function return the variance of the numeric elements of the collection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a link here to the Wikipedia entry that explains the variance. Likewise for the other statistical concepts that follow.

@@ -9,7 +9,7 @@ export default function variance(collection, iteratee, context) {
if (typeof iteratee == 'number' && collection != null && typeof collection[0] != 'object') {
iteratee = null;
}
iteratee = cb(iteratee, context);
iteratee = iteratee(iteratee, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be _.iteratee in order to work.

Suggested change
iteratee = iteratee(iteratee, context);
iteratee = _.iteratee(iteratee, context);

@kpkalwa
Copy link
Author

kpkalwa commented Oct 22, 2021

The document and Function has been updated @jgonggrijp

Copy link
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Excellent! There is only one last thing left to do: remove the Underscore modules from the history of this branch. I'll show you how to achieve this below.

First, make a backup copy of your branch in case anything goes wrong:

git branch statistics-bak statistics

Now we're going to do an interactive rebase. The following command will open an editor:

git rebase -i master statistics

In the editor, you see all of your nine commits listed. On the second, third, fourth and fifth line, change the initial word "pick" into "fixup". This will effectively merge the first five commits into one, causing the Underscore modules that you copied and then removed again to disappear from history.

pick d7d95a2 Statistic Module added
fixup adcd925 Error Fixed
fixup 063e7bd Sum function dependency added and whitespaces issuse resolved
fixup f637c05 Issue in statistics test js fixed
fixup bdd4339 Underscore base package chnages
pick 2a76ba2 Docs added
pick c3aea80 Docs added and Changes made as suggested
pick 72a910b Changes made as suggested

Save your changes and close the editor. Git will now restructure your branch so it consists of four commits and starts from current master. You can check than the overall changes are still the same with the following command (should give no output):

git diff statistics-bak statistics

You can also review your new history in detail with the following command:

git log -p -4

If everything looks good to you, force-push the new version of your branch:

git push --force

You can now remove the backup of your branch:

git branch -D statistics-bak

Alternatively, if anything goes wrong, you can reset your statistics branch to what it was with the following commands:

git checkout statistics
git reset --hard statistics-bak

@kpkalwa
Copy link
Author

kpkalwa commented Oct 25, 2021

Excellent! There is only one last thing left to do: remove the Underscore modules from the history of this branch. I'll show you how to achieve this below.

First, make a backup copy of your branch in case anything goes wrong:

git branch statistics-bak statistics

Now we're going to do an interactive rebase. The following command will open an editor:

git rebase -i master statistics

In the editor, you see all of your nine commits listed. On the second, third, fourth and fifth line, change the initial word "pick" into "fixup". This will effectively merge the first five commits into one, causing the Underscore modules that you copied and then removed again to disappear from history.

pick d7d95a2 Statistic Module added
fixup adcd925 Error Fixed
fixup 063e7bd Sum function dependency added and whitespaces issuse resolved
fixup f637c05 Issue in statistics test js fixed
fixup bdd4339 Underscore base package chnages
pick 2a76ba2 Docs added
pick c3aea80 Docs added and Changes made as suggested
pick 72a910b Changes made as suggested

Save your changes and close the editor. Git will now restructure your branch so it consists of four commits and starts from current master. You can check than the overall changes are still the same with the following command (should give no output):

git diff statistics-bak statistics

You can also review your new history in detail with the following command:

git log -p -4

If everything looks good to you, force-push the new version of your branch:

git push --force

You can now remove the backup of your branch:

git branch -D statistics-bak

Alternatively, if anything goes wrong, you can reset your statistics branch to what it was with the following commands:

git checkout statistics
git reset --hard statistics-bak

The git changes has been updated as per the request @jgonggrijp

@jgonggrijp
Copy link
Contributor

@kpkalwa It seems something went wrong with the rebase. You're now contributing 15 commits rather than the 4 I expected and inspecting the first one confirms that the Underscore modules are still in the history.

Shall I do the rebase or do you want to give it another try? If I do it, the commits will still be credited to you.

@kpkalwa
Copy link
Author

kpkalwa commented Oct 25, 2021

@kpkalwa It seems something went wrong with the rebase. You're now contributing 15 commits rather than the 4 I expected and inspecting the first one confirms that the Underscore modules are still in the history.

Shall I do the rebase or do you want to give it another try? If I do it, the commits will still be credited to you.

You can do the rebase @jgonggrijp

@jgonggrijp jgonggrijp mentioned this pull request Nov 1, 2021
@jgonggrijp
Copy link
Contributor

Replaced by #247.

@jgonggrijp jgonggrijp closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after modules This should be postponed until after modularization (temporary label, see #220) enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants