kscience/kmath
9
0
Fork 1
Code Issues Pull Requests 6 Packages Projects Releases 16 Wiki Activity

Implement hyperbolic functions for various Algebras #118

Merged
CommanderTvis merged 14 commits from hyp-trig-functions into dev 2020-08-11 18:18:06 +03:00
Conversation 1 Commits 14 Files Changed 11 +373 -146
CommanderTvis commented 2020-07-02 20:50:59 +03:00 (Migrated from github.com)
Copy Link
  1. Merge InverseTrigonometric and Trigonometric operations.
  2. Fix division of Complex.
  3. Introduce new interface - HyperbolicTrigonometricOperations.
  4. Add it to ExtendedField composite.
  5. Write KDoc comments for trigonometric functions.
1. Merge InverseTrigonometric and Trigonometric operations. 2. Fix division of Complex. 3. Introduce new interface - HyperbolicTrigonometricOperations. 4. Add it to ExtendedField composite. 5. Write KDoc comments for trigonometric functions.
altavir (Migrated from github.com) requested changes 2020-07-02 21:26:04 +03:00
altavir (Migrated from github.com) left a comment
Copy Link

Need to add tests for hyperbolic functions and check that there is no performance degradation for inlined operations.

Need to add tests for hyperbolic functions and check that there is no performance degradation for inlined operations.
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/Complex.kt
altavir (Migrated from github.com) commented 2020-07-02 21:18:21 +03:00
Copy Link

You are creating additional intermediate Complex value here.

You are creating additional intermediate Complex value here.
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -75,27 +103,24 @@ object RealField : ExtendedField<Double>, Norm<Double, Double> {
override inline fun asin(arg: Double): Double = kotlin.math.asin(arg)
override inline fun atan(arg: Double): Double = kotlin.math.atan(arg)
altavir (Migrated from github.com) commented 2020-07-02 21:20:44 +03:00
Copy Link

Does not make a lot of difference. I think the variant from before was more readable.

Does not make a lot of difference. I think the variant from before was more readable.
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
altavir (Migrated from github.com) commented 2020-07-02 21:21:51 +03:00
Copy Link

Does not make sense to add virtual call here. There is not difference, I guess.

Does not make sense to add virtual call here. There is not difference, I guess.
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
altavir (Migrated from github.com) commented 2020-07-02 21:23:51 +03:00
Copy Link

I do not think that removing this one is free. It could hinder inlining. The same goes for all similar cases. It should not be done without benchmarks.

I do not think that removing this one is free. It could hinder inlining. The same goes for all similar cases. It should not be done without benchmarks.
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
altavir (Migrated from github.com) commented 2020-07-02 21:22:24 +03:00
Copy Link

I like how one-lines look more

I like how one-lines look more
kmath-core/src/commonMain/kotlin/scientifik/kmath/structures/ComplexNDField.kt
altavir (Migrated from github.com) commented 2020-07-02 21:24:59 +03:00
Copy Link

Those should be added to exponential operations and tested

Those should be added to exponential operations and tested
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 09:59:15 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/Complex.kt
CommanderTvis (Migrated from github.com) commented 2020-07-03 09:59:15 +03:00
Copy Link

However, the previous implementation was just invalid

However, the previous implementation was just invalid
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 10:02:19 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
CommanderTvis (Migrated from github.com) commented 2020-07-03 10:02:19 +03:00
Copy Link

The previous property in Java was like:
private int zero = 0; public int getZero() { return zero; }
This one doesn't store zero and doesn't spend any resources to instantiate the value each time because 0 is primitive constant: public int getZero() { return 0; }

The previous property in Java was like: ```private int zero = 0; public int getZero() { return zero; }``` This one doesn't store zero and doesn't spend any resources to instantiate the value each time because `0` is primitive constant: `public int getZero() { return 0; }`
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 10:03:00 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
CommanderTvis (Migrated from github.com) commented 2020-07-03 10:02:47 +03:00
Copy Link

So, this implementation is 4 bytes more efficient than previous :)

So, this implementation is 4 bytes more efficient than previous :)
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 10:03:45 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
CommanderTvis (Migrated from github.com) commented 2020-07-03 10:03:45 +03:00
Copy Link

I'll revert it

I'll revert it
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 10:04:33 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/structures/ComplexNDField.kt
CommanderTvis (Migrated from github.com) commented 2020-07-03 10:04:33 +03:00
Copy Link

OK

OK
altavir (Migrated from github.com) reviewed 2020-07-03 10:05:36 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
altavir (Migrated from github.com) commented 2020-07-03 10:05:36 +03:00
Copy Link

It is a singleton, another 4 bytes do not matter. You have an integer allocation on each call though.

It is a singleton, another 4 bytes do not matter. You have an integer allocation on each call though.
altavir (Migrated from github.com) reviewed 2020-07-03 10:06:09 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/Complex.kt
altavir (Migrated from github.com) commented 2020-07-03 10:06:08 +03:00
Copy Link

Maybe, you need to add a test for it.

Maybe, you need to add a test for it.
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 13:15:06 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
CommanderTvis (Migrated from github.com) commented 2020-07-03 13:15:05 +03:00
Copy Link

No

No
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 13:15:28 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
CommanderTvis (Migrated from github.com) commented 2020-07-03 13:15:28 +03:00
Copy Link

getfield... and iconst_0 produce the equal allocation to the operand stack

`getfield...` and `iconst_0` produce the equal allocation to the operand stack
CommanderTvis (Migrated from github.com) reviewed 2020-07-03 13:15:46 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/Complex.kt
CommanderTvis (Migrated from github.com) commented 2020-07-03 13:15:46 +03:00
Copy Link

Tests are added

Tests are added
altavir (Migrated from github.com) reviewed 2020-07-03 13:16:24 +03:00
kmath-core/src/commonMain/kotlin/scientifik/kmath/operations/NumberAlgebra.kt
@ -212,1 +253,3 @@
override inline fun multiply(a: Long, b: Long): Long = (a * b)
override val zero: Long
get() = 0
altavir (Migrated from github.com) commented 2020-07-03 13:16:24 +03:00
Copy Link

OK, you are correct in case of primitives.

OK, you are correct in case of primitives.
altavir (Migrated from github.com) reviewed 2020-07-27 10:53:00 +03:00
altavir (Migrated from github.com) left a comment
Copy Link

I think that hyperbolic operation should not for their own class, but instead, be a part of exponential operations and have default implementations based on exponents. Otherwise looks good.

I think that hyperbolic operation should not for their own class, but instead, be a part of exponential operations and have default implementations based on exponents. Otherwise looks good.
altavir (Migrated from github.com) approved these changes 2020-08-11 15:30:04 +03:00
altavir commented 2020-08-11 15:30:28 +03:00 (Migrated from github.com)
Copy Link

Please resolve conflicts

Please resolve conflicts
Sign in to join this conversation.
Reviewers
No reviewers
altavir
Labels
Clear labels   
bug

Something isn't working

  
dependencies

Pull requests that update a dependency file

  
discussion

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
feature

A new feature request

  
good first issue

The issue awaits its hero. Contributions are welcome

  
misc

Infrastructure tasks, cosmetic changes and miscellaneous tasks

  
performance

Performance optimization issue

  
question

Further information is requested

  
test

Add tests for some functionality

  
use case

A use case for library or feature

  
wontfix

This will not be worked on

No Label
bug
dependencies
discussion
documentation
duplicate
feature
good first issue
misc
performance
question
test
use case
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: kscience/kmath#118
No description provided.
Delete Branch "hyp-trig-functions"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?