tmhglnd / total-serialism

Toolbox full of Algorithmic Composition methods
MIT License
154 stars 11 forks source link

Add Norgard infinity series generation #27

Closed steve-meyer closed 2 years ago

steve-meyer commented 2 years ago

Added two methods to gen-complex.js for generating the infinity series. Currently only the primary method exports. Not sure if there is a need to expose the norgardInteger() method. The three test script examples demonstrate generating the series with the standard 0,1 seed, an 0,3 seed and generating a sequence starting at index 120 for 8 indices.

I updated the package.json file also because I could not run the test script because it was missing a dependency on the node module "readdirp".

The build files are also updated. Unsure if this is intended for pull requests, but it was required for me to run the tests and verify the output.

tmhglnd commented 2 years ago

Hi Stephen, first of all thanks a lot for the contribution!

Only I'm trying to run your PR, but i'm getting an error when running npm test and am a bit unsure where it is coming from. Error: Cannot find module '/Users/timohoogland/Downloads/total-serialism-infinity-series/node_modules/readdirp/index.js'. Please verify that the package.json has a valid "main" entry You mention in your comment you had to update the package.json with readdirp, i have no idea where this package is coming from and why you needed it? My current version of total-serialism runs fine without it.

I did try to npm i readdirp, but then I run into another error: Error: Cannot find module './lib/stringify'

Do you have any clue where this is coming from? If not, I guess I can just copy your code into my version of total-serialism and not merge this PR.

The build files are also updated. Unsure if this is intended for pull requests, but it was required for me to run the tests and verify the output.

In the test file there is a line on top entryPoint = "../build/ts.es5.min.js";, if you comment this line and save the test file you can just run the test without building because then it will use let entryPoint = "../index"; I realize the testing can be very much improved to make it more user-friendly for future contributors, I'll have a think/look into it. I'm not to much into javascript production of libraries with testing/building etc, so all this is just a patchwork of self-taught things from what I've found online, haha.

Not sure if there is a need to expose the norgardInteger() method

No probably not necessary :)

From what i've just seen now in the code I think I will only swap the order of the arguments. My idea is:

function infinitySeries(size=16, seed=[0,1], offset=0) {}

This will be more consistent with the other generative methods, because all methods that allow to set the length of the output have that as their first argument.

steve-meyer commented 2 years ago

Timo, sorry that I got us into a JavaScript dependency mess! Short version: at this point because there is someone interested (from Max Discord) interested and waiting, maybe the easiest thing to do would be to simply copy the functions from gen-complex.js and the code in the test script.

I just tried to do a completely fresh clone of my fork and branch and am seeing the exact same things you noted above. I'm not remembering exactly how I got things working, but I noted that there were a critical vulnerability warnings. So I think that I attempted to blow away the node_modules directory and rerun npm install. This pulled the latest versions of many modules and removed the most severe vulnerability warnings and also seemed to get everything working.

Here are the steps I did with the fresh clone to get things working just now in case you want to try a quick test before making changes to your version:

$ git clone git@github.com:steve-meyer/total-serialism.git 
$ cd total-serialism
$ git fetch 
$ git branch -v -a 
$ git switch infinity-series 
$ rm -rf node_modules
$ npm install 
$ npm run build 
$ npm test 

If I remember correctly, I was hoping that I could leave the core dependencies alone and just give you the branch/fork with just my relevant functions. I think I had even reverted the changes to the node_modules directory and reran the build/test and it worked. But that seems to be my mistake.

Thanks for the tip about running the tests.

And I agree with your changes to the argument order. Most people will probably assume the [0, 1] seed and only want to specify a sequence length. (In a song I made, I used [0,3] and [0,4] seeds, so it is just the parameter I think about most.) I have always thought the length and offset are also a kind of pairing. Should the offset be second?

Once this is resolved, feel free to ping me if I can help test getting a local development copy up and running. I think one change that might work would be to remove the node_modules directory from the repo. I've seen a lot of recommendations online that adding the dir to .gitignore is recommended.

tmhglnd commented 2 years ago

Hey! So I've just included the code in the latest version 2.2.0. Also added some reference to the docs. Please have a look if you like, and if you feel anything is not correct let me know.

https://www.npmjs.com/package/total-serialism/v/2.2.0

View the reference here:

https://github.com/tmhglnd/total-serialism/blob/master/docs/algorithmic-methods.md#infinityseries

I don't know if the seed or offset will be used more, but I think the seed can lead to more varying results then only changing the offset. So having that as the second argument feels natural to me.

steve-meyer commented 2 years ago

Looks great. Glad this was able to be added.

Closing this pull request as it is no longer needed.