artofscience / SAOR

Sequential Approximate Optimization Repository
GNU General Public License v3.0
5 stars 1 forks source link

Close branches and PR #56

Closed aatmdelissen closed 2 years ago

aatmdelissen commented 3 years ago

The number of branches is getting a bit large. And some pull requests are still open, while they are approved. To prevent merge conflicts and bottle-necks, the branches should be merged and we need to do some cleanup.

TODO

In the future, we should work mainly from the dev branch during development, and once features are tested and seem to be working the dev branch can be merged into main for a new version.

Also, it seems that many pull requests are very large. Try to keep them to a minimum with only a few commits / updated files, only focusing on the feature that you want to implement.

MaxvdKolk commented 3 years ago

I will do my best to resolve any conflicts and merge the PRs that I opened. Unsure when I will get time to take a good look though, there are some deadlines/conferences coming up...

Also think that it is sometimes quite hard to add features, or reorganise parts of the code, as some of the dependencies are "hidden" in example files that are not tested. So, when simply testing if my changes did not break anything, e.g. running pytest. All is OK, but when then manually running scripts from examples/ things break, which makes it hard to quickly add in something small, create a PR, and merge (or rebase).

Giannis1993 commented 3 years ago

Should we then move some of the examples/ to tests/ ? Preferably a simple one that can catch the errors the tests can't (e.g. the solver's issue with the bounds).

I didn't know if the open PRs were finalised, thats why I've left them open. Also, @artofscience added the self-weight problem but it's in the main branch, not in dev. Not sure if he changed other things as well. We should definitely clean up the repo though.

MaxvdKolk commented 3 years ago

Yeah, we could move part of examples to tests/, although I feel that those examples should then assert some behaviour. If the only thing they do is simply run and require the user to inspect if they succeeded or not, it is better to leave them as examples.

If all examples are "essential" to work, we could just make sure we run through all of them manually, or add a little script to the example directory that runs them all, who knows maybe like

#!/bin/bash

for example in ./*.py; do
    python3 "$example"
done

And we could probably enforce in some way that the script runs without running into any errors or so...


But maybe this is all too complicated and I should just run the example files too before trying to merge.

artofscience commented 3 years ago

Closed branch self-weight. Closed issue on dx/dy and ddx/ddy