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

JI op rework #136

Merged
merged 4 commits into from
Jan 25, 2018
Merged

JI op rework #136

merged 4 commits into from
Jan 25, 2018

Conversation

simondemeule
Copy link
Contributor

Removed octave-wrapping behaviour. Now returns exactly the entered
ratio.

Removed octave-wrapping behaviour. Now returns exactly the entered
ratio.
@simondemeule simondemeule changed the title JI rework JI op rework Jan 21, 2018
@simondemeule
Copy link
Contributor Author

See the thread here

@scanner-darkly
Copy link
Member

@tehn @trentgill could you take a look?

@trentgill
Copy link
Contributor

trentgill commented Jan 21, 2018

Code looks good to me.

I think a short note should be added to CHANGELOG.md stating how the op has changed behaviour. Also, I'm not sure if JI is even in the docs yet, but if so, it's description will need to be updated too.

How can we pull the code that generates ji_const[] into the codebase? I had to look at this old PR to find how the numbers were generated, and without it, they're just random magic numbers (see #133).

@scanner-darkly
Copy link
Member

could they be generated with a python script? we could add it to utils folder and put a comment in the code referencing it.

@ngwese
Copy link
Member

ngwese commented Jan 21, 2018

for what its worth it might make sense to just put table generating code right next to the source itself such as was done in libavr32 for the euclidean data table:
https://github.com/monome/libavr32/blob/master/src/euclidean/euclidean.hs

@trentgill
Copy link
Contributor

The generator is really simple (here in C), though uses logf() so it's not created at runtime:
int16_t ji_find_prime_constant( uint16_t prime )
{
float r = 1638.0 * logf( (float)prime ) / log( 2.0 );
r *= 4.0;
return( (int16_t)( r + 0.5 ) );
}

So I think inline would be best. I didn't do that in the first place because JI is already the longest op, and there's no other block comments in the relevant file. If someone wants to translate to Python that's fine by me too.

@scanner-darkly
Copy link
Member

agreed it's best to have it in the source file itself. @simondemeule could you add it to your PR (and also include an update for CHANGELOG.md as mentioned)?

@simondemeule
Copy link
Contributor Author

Here we go!

@simondemeule
Copy link
Contributor Author

Merged because of a conflict in the changelog.

@tehn tehn merged commit 8cbc031 into monome:master Jan 25, 2018
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.

5 participants