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

Keep all_optimizations.py and notebooks up to date with eachother #13

Open
rickyjericevich opened this issue May 18, 2021 · 3 comments
Open
Labels
help wanted Extra attention is needed

Comments

@rickyjericevich
Copy link
Contributor

rickyjericevich commented May 18, 2021

Initially when I created all_optimizations.py, I made sure that the functions for each optimization therein held the exact same code as it's corresponding notebook. Eg: the fte() func had the exact same code as FTE.ipynb. This made it relatively easy to transfer any changes made in the notebooks to all_optimizations.py and vice versa.

The person(s) who recently made changes to all_optimizations.py did not transfer their changes to the notebooks, so the code in all_optimizations.py has become substantially different to the code in the notebooks (and rather messy imo). This makes it difficult and time consuming to add any new changes made in the notebooks to all_optimizations.py and vice versa.

As of right now, all_optimizations.py will probably produce different reconstruction results compared to the notebooks, and that's a problem. This must be fixed before we can merge the improvements from develop into main

I think the best (long-term) solution is to break up the code in each notebook into smaller, more manageable functions and place them in their own module. For example, there'll be one module for all the FTE code and one for EKF and so on. Perhaps the FTE module will have functions like plot_redescending_cost(), initialize_pyomo_model(), define_pyomo_constraints() or something similar.

Thoughts?

@rickyjericevich rickyjericevich added the help wanted Extra attention is needed label May 18, 2021
@DJoska
Copy link
Contributor

DJoska commented May 18, 2021

Agreed, I don't see the benefit in having the optimisation functions defined twice (once for all_optimisations and once for each separate notebook). Creating new modules and simply referring to those makes sense. Perhaps then we could also just have one notebook that shows how to call each module for SBA, FTE and EKF instead of having three separate notebooks.

@rickyjericevich I think let's make these changes after we have had the call to discuss the other bugs, so we can start the revision with the cleanest code possible.

@MMathisLab
Copy link
Collaborator

If useful, I'd be happy to chat about ways to optimize the API. I would strongly suggest adopting simple wrapper functions for every step such that acinoset can be packaged as used in a similar way to deeplabcut?

@DenDen047
Copy link
Collaborator

I also agree with the integration of each algorithm code and love the simple wrapper functions. The directory structure in DeepLabCut is useful for our directory design, such as https://github.com/DeepLabCut/DeepLabCut/tree/master/deeplabcut/pose_estimation_tensorflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants