Reducing complexity on a number of classes. #2480
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 12 commits
August 31, 2019 12:38
Only cosmetic and readability improvements. No functional changes were intended. Utilities.cs - Fixed comments across file - Made class static - Removed unnecessary imports - Removed unused method arguments - Renamed variables as appropriate to make usage clearer - In AddRangeNoAlloc, disabled (by comment) Rider’s suggestion to revert to use of built-in Range field (Fixed) - In TextureToTensorProxy, swapped order of first two arguments to be more in-line with convention of input, output UtilitiesTests.cs - Removed unnecessary imports - Simplified array creation commands GeneratorImp.cs - Rider automatically deleted spaces on empty lines - Changed call to TextureToTensorProxy to mirror new argument ordering
- Removed unnecessary imports - Fixed comment warning - Fixed method header
- Created const for SCENE_NAME field - Fixed string formatting for exception message - Added new line at EOF
- Removed FillTensor from RandomNormal and moved to TensorUtils - Removed the tests in RandomNormalTest that are for FillTensor to TensorUtilsTest - Fixed GeneratorImpl to use to TensorUtils.FillTensor method
- Add use of var where appropriate - Makes variables readonly where appropriate - Makes private variables start with “_” - Removes DiscreteActionOutputApplier context from Multinomial - class is much simpler now - Moves most of the tests in MultinomialTest to a new DiscreteActionOutputApplierTest class - Adds a basic Multinomial test to MultinomialTest
- Limiting code to 100 characters - renaming private vars to start with “_” - fixing indentation in file Couple of other minor fixes to TensorUtilsTest and Discrete…Test.cs
Changed Name —> name; ValueType —> valueType; Shape —> shape; Data —> data
- Limit to 100 chars per line - Appropriate formatting - Removed Rider warnings
|
|
vincentpierre
approved these changes
Sep 4, 2019
| @@ -0,0 +1,5 @@ | |||
| <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> | |||
There was a problem hiding this comment.
What is this file for ?
There was a problem hiding this comment.
Yes, this Rider file includes words added to the Rider dictionary.
| @@ -194,14 +197,15 @@ public void Generate(TensorProxy tensorProxy, int batchSize, Dictionary<Agent, A | |||
| /// </summary> | |||
| public class PreviousActionInputGenerator : TensorGenerator.Generator | |||
| { | |||
| private ITensorAllocator _allocator; | |||
| private readonly ITensorAllocator _allocator; | |||
There was a problem hiding this comment.
You should ask a review to Mantas about this
| @@ -1,6 +1,5 @@ | |||
| using System; | |||
There was a problem hiding this comment.
I would prefer to keep those for reference.
chriselion
reviewed
Sep 4, 2019
| }; | ||
| var batchSize = 4; | ||
| const int batchSize = 4; |
There was a problem hiding this comment.
What's the general guideline for when to use var? You move some things to it, and others (like here) away from it.
There was a problem hiding this comment.
The general guideline is to use it when the variable left type matches the right type.
Fixed the tests for Multinomial as they didn’t include cumulative mass functions. Also, improved the documentation.
|
@surfnerd / @vincentpierre - realized that my editor tests weren't passing, made the appropriate fix to tests and added relevant documentation. |
|
changes look good |
surfnerd
pushed a commit
that referenced
this pull request
Sep 12, 2019
Only cosmetic and readability improvements. No functional changes were intended. Utilities.cs - Fixed comments across file - Made class static - Removed unnecessary imports - Removed unused method arguments - Renamed variables as appropriate to make usage clearer - In AddRangeNoAlloc, disabled (by comment) Rider’s suggestion to revert to use of built-in Range field (Fixed) - In TextureToTensorProxy, swapped order of first two arguments to be more in-line with convention of input, output UtilitiesTests.cs - Removed unnecessary imports - Simplified array creation commands GeneratorImp.cs - Rider automatically deleted spaces on empty lines - Changed call to TextureToTensorProxy to mirror new argument ordering * Clean-up to UnityAgentsException.cs - Removed unnecessary imports - Fixed comment warning - Fixed method header * Improvements to Startup.cs - Created const for SCENE_NAME field - Fixed string formatting for exception message - Added new line at EOF * Adding team-wide settings file * Clean up to RandomNormal - Removed FillTensor from RandomNormal and moved to TensorUtils - Removed the tests in RandomNormalTest that are for FillTensor to TensorUtilsTest - Fixed GeneratorImpl to use to TensorUtils.FillTensor method * Decouples DiscreteActionOutputApplier from Multinomial - Add use of var where appropriate - Makes variables readonly where appropriate - Makes private variables start with “_” - Removes DiscreteActionOutputApplier context from Multinomial - class is much simpler now - Moves most of the tests in MultinomialTest to a new DiscreteActionOutputApplierTest class - Adds a basic Multinomial test to MultinomialTest * Minor formatting to TensorProxy - Limiting code to 100 characters - renaming private vars to start with “_” - fixing indentation in file Couple of other minor fixes to TensorUtilsTest and Discrete…Test.cs * Adding new line to EOF for TensorProxy * Adding missing meta files. * Removing three unused methods from TensorProxy * (Big) Rename of TensorProxy fields Changed Name —> name; ValueType —> valueType; Shape —> shape; Data —> data * Additional cosmetic changes to a number of files previously modified - Limit to 100 chars per line - Appropriate formatting - Removed Rider warnings * Improvements to Multinomial(Test).cs Fixed the tests for Multinomial as they didn’t include cumulative mass functions. Also, improved the documentation.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are no functional changes in any of the modifications in this PR. The individual commit messages contain detailed descriptions of each incremental commit.