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

code-review feedback #1

Open
orso82 opened this issue Oct 23, 2024 · 0 comments
Open

code-review feedback #1

orso82 opened this issue Oct 23, 2024 · 0 comments
Assignees

Comments

@orso82
Copy link
Member

orso82 commented Oct 23, 2024

Fantastic work @mgyoo86 I am truly impressed at how quickly you put this together and I appreciate the high-quality printouts and plots! 🥇

I have some feedback mostly based on my experience developing Julia code. I hope you find my comments useful. The code is already very-much usable as is, but I think we can make it even better!

Style

  1. Typically julia functions start with lower case. Upper casing is usually reserved for structs.
  2. You could consider splitting your code into multiple files

Abstraction

  1. You could define a BetaLimitModel abstract type, with MLP_Model and CNN_Model be concrete types of it. This will make working with the MLP and CNN models more transparent, allowing in certain situations to dispatch on the abstract type instead of the concrete types.

Plotting

  1. You should take a look at Plots.jl recipes, which allow you to define how plot() should behave on a specific type. In your case @recipie plot(sampPoints::Sample_Points), @recipie plot(model::BetaLimitModel). Recipes are very powerful, once you get around using them, there's no turning back! ;) Also, people don't have to remember the name of individual plot functions, they just apply plot() to different objects!

Printing

  1. Instead of the different _print_results_to_stdout methods, you can define your Base.show(io::IO, ::MIME"plain/text", model::BetaLimitModel) which will allow you to get the fancy prints whenever a model is returned in a Jupyter cell.

Performance

  1. It seems you are re-loading the models each time the limits are calculated. That's very inefficient. You can load each model only once and then keep re-using them (you can use Memoize.jl or caching the models yourself).

Saving results to dd

  1. we need to figure out where βₙ_limit should be stored in the dd. Once βₙ_limit is in the dd there will be no reason to have models for each time slice. Perhaps @bclyons12 can comment on this one.
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

No branches or pull requests

2 participants