update harmony to new implementation #308
Conversation
|
I’ll check this out tomorrow, it’s too big to start now! |
flying-sheep
left a comment
flying-sheep
left a comment
There was a problem hiding this comment.
The harmonize function has a really nice layout. it’s easy to follow what it does, nice!
However it seems like you’re establishing a bunch of parameters that get reused and never changed after the initialization. You could e.g.
- create it a frozen dataclass with methods so you can use the parameters using
self.<name>, or - create a
NamedTuplethat you can pass around containing the parameters
There was a problem hiding this comment.
- don’t name variables LIKE_CONSTANTS
- don’t name variables with single letter names (except for
a, bfor binary operators,iforenumerateand similar conventions)
There was a problem hiding this comment.
the variables still have weird names with capital letters in them
Co-authored-by: Philipp A. <[email protected]>
Co-authored-by: Philipp A. <[email protected]>
|
removed all prints and added a warning if harmony didnt converge |
flying-sheep
left a comment
flying-sheep
left a comment
There was a problem hiding this comment.
Please also check the review summary: #308 (review)
There was a problem hiding this comment.
the variables still have weird names with capital letters in them
|
Most of the names are just taken from the reference implementation |
flying-sheep
left a comment
flying-sheep
left a comment
There was a problem hiding this comment.
You said you’re going to do a follow-up PR that moves all the various reused parameters (Phi, R, E, O, theta, …) into a container. Otherwise looks good!
Fixes #299