Sourcery Starbot ⭐ refactored brendanator/atari-rl #27
Conversation
| elif config.async == 'n_step': | ||
| config.batch_size = 1 | ||
| elif config.async == 'a3c': | ||
| elif config. async in ['n_step', 'a3c']: | ||
| config.batch_size = 1 | ||
| else: | ||
| raise Exception('Unknown asynchronous algorithm: ' + config.async) | ||
| raise Exception(f'Unknown asynchronous algorithm: {config.async}') |
There was a problem hiding this comment.
Function create_config refactored with the following changes:
- Merge duplicate blocks in conditional (
merge-duplicate-blocks) - Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Remove redundant conditional (
remove-redundant-if) - Replace multiple comparisons of same variable with
inoperator (merge-comparisons)
| # Epsilon greedy exploration/exploitation even for bootstrapped DQN | ||
| if np.random.rand() < self.epsilon(step): | ||
| return self.atari.sample_action() | ||
| else: | ||
| [action] = session.run( | ||
| self.policy_network.choose_action, | ||
| {self.policy_network.inputs.observations: [observation]}) | ||
| return action | ||
| [action] = session.run( | ||
| self.policy_network.choose_action, | ||
| {self.policy_network.inputs.observations: [observation]}) | ||
| return action |
There was a problem hiding this comment.
Function Agent.action refactored with the following changes:
- Remove unnecessary else after guard condition (
remove-unnecessary-else)
This removes the following comments ( why? ):
# Epsilon greedy exploration/exploitation even for bootstrapped DQN
| if pseudo_count < 0: | ||
| pseudo_count = 0 # Occasionally happens at start of training | ||
|
|
||
| # Return exploration bonus | ||
| exploration_bonus = self.beta / math.sqrt(pseudo_count + 0.01) | ||
| return exploration_bonus | ||
| pseudo_count = max(pseudo_count, 0) | ||
| return self.beta / math.sqrt(pseudo_count + 0.01) |
There was a problem hiding this comment.
Function ExplorationBonus.bonus refactored with the following changes:
- Replace comparison with min/max call (
min-max-identity) - Inline variable that is immediately returned (
inline-immediately-returned-variable)
This removes the following comments ( why? ):
# Return exploration bonus
# Occasionally happens at start of training
| raise Exception('Unknown replay_priorities: ' + config.replay_priorities) | ||
| raise Exception(f'Unknown replay_priorities: {config.replay_priorities}') |
There was a problem hiding this comment.
Function ReplayMemory.__init__ refactored with the following changes:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation)
| name = self.run_dir + 'replay_' + threading.current_thread().name + '.hdf' | ||
| name = f'{self.run_dir}replay_{threading.current_thread().name}.hdf' |
There was a problem hiding this comment.
Function ReplayMemory.save refactored with the following changes:
- Use f-string instead of string concatenation [×3] (
use-fstring-for-concatenation)
| discounted_reward = sum([ | ||
| reward * config.discount_rate**(reward - 4) for reward in range(4, 11) | ||
| ]) | ||
| discounted_reward = sum(reward * config.discount_rate**(reward - 4) | ||
| for reward in range(4, 11)) |
There was a problem hiding this comment.
Function ReplayMemoryTest.test_replay_memory refactored with the following changes:
- Replace unneeded comprehension with generator (
comprehension-to-generator)
| if self.run_summary(step): | ||
| return [self.summary_op] | ||
| else: | ||
| return [self.dummy_summary_op] | ||
| return [self.summary_op] if self.run_summary(step) else [self.dummy_summary_op] |
There was a problem hiding this comment.
Function Summary.operation refactored with the following changes:
- Replace if statement with if expression (
assign-if-exp)
| return max([int(run) for run in runs]) | ||
| return max(int(run) for run in runs) | ||
|
|
||
| return 0 | ||
|
|
||
| if config.run_dir == 'latest': | ||
| parent_dir = 'runs/%s/' % config.game | ||
| parent_dir = f'runs/{config.game}/' | ||
| previous_run = find_previous_run(parent_dir) | ||
| run_dir = parent_dir + ('run_%d' % previous_run) | ||
| elif config.run_dir: | ||
| run_dir = config.run_dir | ||
| else: | ||
| parent_dir = 'runs/%s/' % config.game | ||
| parent_dir = f'runs/{config.game}/' |
There was a problem hiding this comment.
Function run_directory refactored with the following changes:
- Replace unneeded comprehension with generator (
comprehension-to-generator) - Replace interpolated string formatting with f-string [×3] (
replace-interpolation-with-fstring)
| return prefix + '_t_plus_' + str(t) | ||
| return f'{prefix}_t_plus_{str(t)}' | ||
| elif t == 0: | ||
| return prefix + '_t' | ||
| return f'{prefix}_t' | ||
| else: | ||
| return prefix + '_t_minus_' + str(-t) | ||
| return f'{prefix}_t_minus_{str(-t)}' |
There was a problem hiding this comment.
Function format_offset refactored with the following changes:
- Use f-string instead of string concatenation [×5] (
use-fstring-for-concatenation)
| print('%s %s: %s' % (now, thread_id, message)) | ||
| print(f'{now} {thread_id}: {message}') |
There was a problem hiding this comment.
Function log refactored with the following changes:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring)
WalkthroughThe recent modifications across various components of the codebase focus on enhancing readability, maintainability, and efficiency. Changes include the adoption of f-strings for string formatting, the use of conditional and generator expressions for more concise code, and minor logic adjustments for robustness. These updates streamline the code, making it easier to understand and work with, without altering the core functionality or control flow. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Refactoring
PR Summary: The pull request consists of refactoring changes aimed at improving code readability and efficiency. It includes the use of f-strings for string formatting, simplification of conditional statements, and the use of list comprehensions and other Pythonic idioms. The changes also involve cleaning up the code by removing unnecessary else blocks after return statements and using more concise expressions for conditional assignments.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the refactoring changes do not alter the intended logic of the code, especially in conditional statements where the order of evaluation matters.
- Verify that the use of f-strings and other modern Python features is consistent across the entire codebase to maintain readability and style consistency.
- Review the changes for any potential syntax errors introduced during refactoring, such as the extra space found in 'config. async'.
- Consider running the full test suite to ensure that the refactored code behaves as expected and that no regressions have been introduced.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -81,15 +81,14 @@ def train_agent(self, session, agent): | |||
| agent.replay_memory.save() | |||
|
|
|||
| def reset_target_network(self, session, step): | |||
| if self.reset_op: | |||
| if step > 0 and step % self.config.target_network_update_period == 0: | |||
| if step > 0 and step % self.config.target_network_update_period == 0: | |||
There was a problem hiding this comment.
question (llm): The refactoring to combine the conditions into a single if statement is good for readability, but ensure that the logic is equivalent and that self.reset_op is always defined when needed.
| elif config.async == 'n_step': | ||
| config.batch_size = 1 | ||
| elif config.async == 'a3c': | ||
| elif config. async in ['n_step', 'a3c']: |
There was a problem hiding this comment.
issue (llm): There's an extra space between 'config.' and 'async' which could lead to a syntax error. This should be corrected.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (13)
- agents/agent.py (1 hunks)
- agents/exploration_bonus.py (1 hunks)
- agents/replay_memory.py (3 hunks)
- agents/training.py (1 hunks)
- atari/atari.py (1 hunks)
- main.py (1 hunks)
- networks/dqn.py (1 hunks)
- networks/factory.py (1 hunks)
- networks/inputs.py (3 hunks)
- networks/reward_scaling.py (1 hunks)
- test/test_replay_memory.py (1 hunks)
- util/summary.py (1 hunks)
- util/util.py (3 hunks)
Files skipped from review due to trivial changes (5)
- agents/replay_memory.py
- atari/atari.py
- networks/dqn.py
- util/summary.py
- util/util.py
Additional comments: 11
agents/exploration_bonus.py (1)
- 23-24: The modification to use
max(pseudo_count, 0)for ensuringpseudo_countis non-negative is a good practice for simplifying logic and improving code readability.test/test_replay_memory.py (1)
- 50-51: Changing from a list comprehension to a generator expression within the
sumfunction is a performance optimization that avoids creating an intermediate list. Good job on this improvement.networks/reward_scaling.py (1)
- 34-34: Simplifying the return statement in the
batch_sigma_squaredmethod to directly returnsigma_squaredif it's greater than 0, otherwise 1.0, is a clean and efficient approach.agents/agent.py (1)
- 27-30: Removing the epsilon greedy exploration/exploitation logic and always using the policy network to choose an action simplifies the agent's decision-making process. This aligns with the objective of simplifying logic.
agents/training.py (2)
- 84-85: Directly checking if
stepis greater than 0 and a multiple ofself.config.target_network_update_periodbefore resetting the target network is a clear and efficient approach.- 91-91: Using the walrus operator for conditional assignment in the
train_batchmethod is a modern Python practice that improves code readability and efficiency.networks/factory.py (1)
- 111-111: Updating string formatting from concatenation to f-string in the
create_summary_opsmethod enhances readability and performance. Good use of modern Python features.networks/inputs.py (3)
- 69-69: Using a formatted string for the
nameparameter in thetf.placeholder_with_defaultcall improves readability and is a good use of modern Python features.- 101-101: Initializing the
feedsattribute in theRequiredFeedsclass using a conditional expression simplifies the logic and improves code readability.- 150-166: Refactoring the
required_feedsmethod to handle input tensors and cache results more efficiently is a good practice for improving code performance and maintainability.main.py (1)
- 158-161: Consolidating the logic for setting
batch_sizein thecreate_configfunction into a singleif-elif-elseblock simplifies the control flow and improves clarity. Using an f-string for the exception message is also a readability improvement.
|
@sourcery-ai review |
Thanks for starring sourcery-ai/sourcery ✨ 🌟 ✨
Here's your pull request refactoring your most popular Python repo.
If you want Sourcery to refactor all your Python repos and incoming pull requests install our bot.
Review changes via command line
To manually merge these changes, make sure you're on the
mainbranch, then run:Summary by CodeRabbit