search

dolphin-acoustics-vip / artwarp

MATLAB program for automated categorisation of tonal animal sounds
https://github.com/dolphin-acoustics-vip/artwarp/wiki
GNU Lesser General Public License v3.0
6 stars 9 forks source link

Comments for next semester's dev team, implement clearer naming conventions #49

Closed lixitrixi closed 1 year ago

lixitrixi commented 1 year ago

Michael and I discussed possible Network structures, and added comments to reflect ARTwarp goals for those working on the project next year.

olexandr-konovalov commented 1 year ago

@lixitrixi please investigate failing unit tests

lixitrixi commented 1 year ago

I will wait for @michaelstanway's recent pull request to be merged, then add the comments from this commit to the changed files to avoid unnecessary conflicts.

@olexandr-konovalov

harvard-tham commented 1 year ago

the error is a simple change in the unit test from weight to weights. Can i checkout this or does it have to be Felix that changes it.

harvard-tham commented 1 year ago

found out how to push to your fork, should work now

olexandr-konovalov commented 1 year ago

@lixitrixi requested changes - would be nice to address them and merge this ASAP. I don't think #50 is quite ready yet.

By the way, use the #<PR number> syntax for cross-referencing, rather than saying "@michaelstanway's recent pull request".

olexandr-konovalov commented 1 year ago

@lixitrixi P.S. and of course if the CLI code is runnable, the CI already is ready to run it on GitHub actions. If not runnable, then not mergeable. So that should be really a separate PR. Would you like me to refactor it, or would you do it yourself? You need to create a new branch, cherry-pick the CLI commit there, then go back to the branch for this PR and rebase it to remove the CLI commit. BTW, you again submit a PR from your rewrite, forgetting to create a feature branch - one more argument to push for merging rewrite into main, but we can't do it as we are keeping adding stuff and it becomes hard to sync moving targets.

olexandr-konovalov commented 1 year ago

What's the plan here? Unless anyone wishes to do this, I will take out @lixitrixi's CLI into a separate unmerged PR, and squash and merge the rest.

After that, I am going to merge #46 into main, so that next time we start off the main branch.

lixitrixi commented 1 year ago

@olexandr-konovalov I apologize for the problems, I will place the CLI code into a separate PR as soon as I can. I am currently settling back into life at home, but I will find some time this week! I the CLI is completely separate, so it will be easy to make them separate.

olexandr-konovalov commented 1 year ago

@lixitrixi thanks, sounds good, take your time then - will look here in a couple of weeks!

lixitrixi commented 1 year ago

@olexandr-konovalov Sorry for the extended hiatus...

I have removed CLI-related files from this PR, and will add them to a separate PR after refactoring and making the code more complete. Just to confirm, you mean that I should create a separate feature branch on my fork for CLI code? As in: dolphin-acoustics-vip:rewrite <- lixitrixi:CLI-code (or similar) ?

lixitrixi commented 1 year ago

The PR adding CLI code is ready on my fork, just waiting on this to be merged since that PR currently includes all the changes in this one as well.

olexandr-konovalov commented 1 year ago

Thanks @lixitrixi - there could be a better way to refactor the PR then deleting files, but to save time I have just squashed and merge it instead of another refactoring.

olexandr-konovalov commented 1 year ago

@lixitrixi now #46 merged and you can proceed with adding CLI. However, I am afraid that it's now based on the wrong branch. From now on, contributions are done not to the rewrite branch but to main (easier). For any change, pull the latest state of main, create a branch off it, add a change, submit a PR from it.