-
Notifications
You must be signed in to change notification settings - Fork 1
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
ihategit #33
base: dev
Are you sure you want to change the base?
ihategit #33
Conversation
Much better results. Empty and Cluttered songs have been almost eliminated (used to be a majority with my data).
reduced overfitting
Sourcery refactored MEinkitten branch
Configure Sweep
Sweep: Convert to Pytorch
…o_pytorch Sweep: Convert new master branch to pytorch
Update README.md
Much better results. Empty and Cluttered songs have been almost eliminated (used to be a majority with my data). Co-authored-by: TheLastLionTurtle <[email protected]>
* Added noise in input Much better results. Empty and Cluttered songs have been almost eliminated (used to be a majority with my data). * Added noise layer reduced overfitting --------- Co-authored-by: TheLastLionTurtle <[email protected]>
migration sync w pk
Missed chanegs from last PR
Einhorn upgrades
add back noise_rate
Einhorn -> picklekitten
merge old updates
Code cleaning
restore prior Pytorch migration
modified: .gitignore new file: tests.py modified: train.py
…to picklekitten * 'picklekitten' of https://github.com/aleph23/Composer: Delete .vscode directory Delete .vs directory Delete .idea directory Update sweep-template.yml (#32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aleph23 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 9 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -6,24 +6,31 @@ | |||
""" | |||
|
|||
import argparse | |||
import itertools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consider removing unused import if 'itertools' is not utilized in the module.
import itertools | |
import itertools |
@@ -251,7 +263,7 @@ def update_mouse_click(mouse_pos): | |||
mouse_pressed = 2 | |||
|
|||
|
|||
def apply_controls(): | |||
def apply_controls() -> object: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_clarification): Explicit return type hint 'object' is too generic; specify a more precise type if possible.
def apply_controls() -> object: | |
def apply_controls() -> None: |
@@ -0,0 +1,476 @@ | |||
{ | |||
"cells": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consider using a more descriptive name for the notebook file.
The name 'GradiantVis.ipynb' could be a typo or may not fully describe the purpose of the notebook. Consider renaming it to something more descriptive, such as 'GradientVisualization.ipynb', if it is indeed about visualizing gradients.
"from keras.models import Model, load_model\n", | ||
"from keras import backend as K\n", | ||
"import numpy as np\n", | ||
"import params\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code_refinement): Avoid duplicate imports.
The module 'params' is imported twice. Removing duplicate imports can clean up the code and improve its readability.
"# User constants\n", | ||
"dir_name = 'results/history/'\n", | ||
"sub_dir_name = 'basic'\n", | ||
"num_measures = 16\n", | ||
"use_pca = True" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_refinement): Consider using a configuration file or environment variables for constants.
Hardcoding constants in the script can lead to difficulties in changes or different environments. Using a configuration file or environment variables can make the code more flexible and easier to manage across different environments or conditions.
"# User constants\n", | |
"dir_name = 'results/history/'\n", | |
"sub_dir_name = 'basic'\n", | |
"num_measures = 16\n", | |
"use_pca = True" | |
import json | |
# Load configuration | |
with open('config.json', 'r') as config_file: | |
config = json.load(config_file) | |
dir_name = config['dir_name'] | |
sub_dir_name = config['sub_dir_name'] | |
num_measures = config['num_measures'] | |
use_pca = config['use_pca'] |
if samples: # If there are samples, check their structure | ||
assert isinstance(samples[0], np.ndarray), f"Test ID {test_id}: Each sample should be a numpy array." | ||
assert samples[0].shape == (samples_per_measure, num_notes), f"Test ID {test_id}: Incorrect shape of sample." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
@@ -101,6 +99,7 @@ def plot_losses(scores, f_name, on_top=True): | |||
|
|||
|
|||
def save_training_config(num_songs, model, learning_rate): | |||
# sourcery skip: use-fstring-for-concatenation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Unused skip comment (unused-skip-comment
)
@@ -117,7 +124,7 @@ | |||
audio_pause = False | |||
|
|||
|
|||
def audio_callback(in_data, frame_count, time_info, status): | |||
def audio_callback(in_data,frame_count,time_info,status): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): We've found these issues:
- Simplify conditional into switch-like form (
switch
) - Extract code out into function (
extract-method
) - Low code quality found in audio_callback - 10% (
low-code-quality
)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
||
self.clock = pygame.time.Clock() | ||
|
||
def update(self, events): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Low code quality found in TextInput.update - 15% (low-code-quality
)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
deleted: .vs/VSWorkspaceState.json deleted: .vscode/settings.json
No description provided.