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

New math functions #253

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

awu1130
Copy link

@awu1130 awu1130 commented Oct 2, 2024

Includes changes related to Issue: #10

Included new maths functions and tests to calculate the geometric mean, triangle inequality, coprime, binary exponentiation, and area of regular hexagon, pentagon, and octagon, plus the tests for each.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

  • All the area calculation functions are algorithmically pretty uninteresting formulae and should thus be removed IMO. Same for the geometric mean, though that might be more useful. I'm slightly on the fence about the triangle inequality checking; it's probably fine either way.
  • Please get rid of redundant @function doc comments.
  • Please get rid of unrelated changes: package.json, package-lock.json

/**
* @function binaryExponent
* @description Returns the power of a number A raised to the power of B with binary exponentiation
* @summary Binary exponentiation calculatoes A^B in logarithmic time of B instead of linear time
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
* @summary Binary exponentiation calculatoes A^B in logarithmic time of B instead of linear time
* @summary Binary exponentiation calculates A^B in time O(log B) instead of O(B)

* @function binaryExponent
* @description Returns the power of a number A raised to the power of B with binary exponentiation
* @summary Binary exponentiation calculatoes A^B in logarithmic time of B instead of linear time
* @param {Number} num - An array of two natural numbers, [A, B], where A^B will be solved
Copy link
Contributor

Choose a reason for hiding this comment

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

Just take two numbers, base and exponent, as parameters instead.

* @summary Binary exponentiation calculatoes A^B in logarithmic time of B instead of linear time
* @param {Number} num - An array of two natural numbers, [A, B], where A^B will be solved
* @return {number} A^B
* @see [Wikipedia](https://cp-algorithms.com/algebra/binary-exp.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not Wikipedia.

throw new TypeError('Invalid Input')
}
for(let i=0; i < numbers.length; i++){
if (numbers[i] < 0 || !Number.isInteger(numbers[i])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The base can very well be negative. You could also support negative exponents but you don't have to.

export const binaryExponent = (numbers: number[]): number => {
// raise errors upon invalid inputs
if (numbers.length != 2) {
throw new TypeError('Invalid Input')
Copy link
Contributor

Choose a reason for hiding this comment

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

This becomes obsolete if you fix the signature.

console.log(factors1)

// check if same factors
for (let i = 0; i <= factors0.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be optimized e.g. by doing a sorted merge instead. This is O(nm) but could be O(n + m). Not really necessary since the factorization is more expensive though (which should be noted however).

import { binaryExponent } from '../binary_exponentiation'

describe('Tests for HexArea', () => {
it('should be a function', () => {
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 unnecessary.

@@ -0,0 +1,29 @@
import { binaryExponent } from '../binary_exponentiation'

describe('Tests for HexArea', () => {
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
describe('Tests for HexArea', () => {
describe('HexArea', () => {

@@ -0,0 +1,25 @@
import { coPrime } from '../co_prime'

describe('Tests for CoPrime', () => {
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
describe('Tests for CoPrime', () => {
describe('CoPrime', () => {

import { coPrime } from '../co_prime'

describe('Tests for CoPrime', () => {
it('should be a function', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@appgurueu
Copy link
Contributor

Please remove the coprime check: #250 was opened earlier and implements a strictly better algorithm, obsoleting this one.

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.

2 participants