Use symlink for output_template/python/lmnet/utils/output.py #688

yd8534976 · 2019-12-18T07:39:01Z

Related to: #684 .

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature / Optimization (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

bo-code-review-bot · 2019-12-20T04:09:05Z

This PR needs Approvals as follows.

Ownership Approval for / from iizukak, tkng, ruimashita
Readability Approval for Python from tkng, tsawada

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

Approve all reviews requested to you (readability and ownership) and LGTM review
Approval, LGTM
Approve all ownership reviews
Ownership Approval or OA
Approve all readability reviews
Readability Approval or RA
Approve specified review targets
- Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
- Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python
Approve LGTM review
LGTM

See all trigger comments

Please replace [Target] to review target

Ownership Approval
- Ownership Approval for [Target]
- OA for [Target]
- Ownership Approval
- OA
- Approval
Readability Approval
- Readability Approval for [Target]
- RA for [Target]
- [Target] Readability Approval
- [Target] RA
- Readability Approval
- RA
- Approval
LGTM
- LGTM
- lgtm

tsawada · 2019-12-23T08:10:40Z

Please put me back after ownership review.

iizukak

Ownership Approval

tsawada

Do we need these diffs in predict_output/output.py in order to replace output_template/.../output.py with a symlink?

yd8534976 · 2019-12-23T08:30:44Z

@tsawada , Thanks!
Since there are some differences between two files.

For example, we have no matplotlib in output_template/requirements.txt.

blueoil/lmnet/lmnet/utils/predict_output/output.py

Line 25 in 02598dd

from matplotlib import cm

blueoil/output_template/python/requirements.txt

Lines 1 to 9 in 02598dd

    
           click==6.7 
        
           easydict==1.6 
        
           numpy==1.14.0 
        
           Pillow==6.2.1 
        
           PyYAML==4.2b4 
        
           # for python2 
        
           enum34==1.1.6 
        
           six==1.11.0

There is no bench in predict_output/output.py's __init__().

blueoil/output_template/python/lmnet/utils/output.py

Lines 36 to 42 in 02598dd

    
           def __init__(self, task, classes, image_size, data_format, bench={}): 
        
               assert task in Tasks 
        
               self.task = task 
        
               self.classes = classes 
        
               self.image_size = image_size 
        
               self.data_format = data_format 
        
               self.bench = {}

blueoil/lmnet/lmnet/utils/predict_output/output.py

Lines 38 to 43 in 02598dd

    
           def __init__(self, task, classes, image_size, data_format): 
        
               assert task in Tasks 
        
               self.task = task 
        
               self.classes = classes 
        
               self.image_size = image_size 
        
               self.data_format = data_format

So I synchronized it manually.

tsawada · 2019-12-24T02:20:52Z

lmnet/lmnet/utils/predict_output/output.py

@@ -35,12 +34,13 @@ class JsonOutput():
    Please see [Output Data Specification](https://github.com/LeapMind/lmnet/wiki/Output-Data-Specification).
    """

-    def __init__(self, task, classes, image_size, data_format):
+    def __init__(self, task, classes, image_size, data_format, bench={}):


Don't use mutable object as an argument's default value
http://go/python-review-guidelines#using-listdict-as-the-default-arg

Thanks, it is reasonable. Fixed.

tsawada · 2019-12-24T02:22:11Z

lmnet/lmnet/utils/predict_output/output.py

 from io import BytesIO

 import numpy as np
 import PIL.Image
 import PIL.ImageDraw
-from matplotlib import cm


can we remove matplotlib from requirements.txt?

After this PR, only lmnet/networks/classification/base.py will depend on matplotlib.

blueoil/lmnet/lmnet/networks/classification/base.py

Line 17 in 8055a4a

from matplotlib import cm

Is it worth adding a new dependency for visualization? If not, I would remove it in another PR and reimplement this.
Related to: #485

I see. I think it'd be nice.

tsawada

Readability Approval

tsawada · 2019-12-24T04:41:10Z

lmnet/lmnet/utils/predict_output/output.py

 from io import BytesIO

 import numpy as np
 import PIL.Image
 import PIL.ImageDraw
-from matplotlib import cm


I see. I think it'd be nice.

Init symlink for output.py

7f74306

blueoil-butler bot added the CI: auto-run Run CI automatically label Dec 18, 2019

Sync up output.py

c8839ee

yd8534976 changed the title ~~[WIP] Use symlink for output_template/python/lmnet/utils/output.py~~ Use symlink for output_template/python/lmnet/utils/output.py Dec 20, 2019

yd8534976 requested review from iizukak, tsawada, tkng and kchygoe December 20, 2019 04:09

tsawada removed their request for review December 23, 2019 08:10

iizukak approved these changes Dec 23, 2019

View reviewed changes

yd8534976 requested a review from tsawada December 23, 2019 08:13

tsawada reviewed Dec 23, 2019

View reviewed changes

tsawada reviewed Dec 24, 2019

View reviewed changes

Change default parameter {} to None

10b8683

tsawada approved these changes Dec 24, 2019

View reviewed changes

yd8534976 added 3 commits December 26, 2019 12:37

Resolve conflicts

215e57c

Merge branch 'master' into symbolic_link_for_output

4805469

Merge branch 'master' into symbolic_link_for_output

2be0657

yd8534976 merged commit e156424 into blue-oil:master Dec 26, 2019

yd8534976 deleted the symbolic_link_for_output branch December 26, 2019 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use symlink for output_template/python/lmnet/utils/output.py #688

Use symlink for output_template/python/lmnet/utils/output.py #688

yd8534976 commented Dec 18, 2019 •

edited

Loading

bo-code-review-bot bot commented Dec 20, 2019

tsawada commented Dec 23, 2019

iizukak left a comment

tsawada left a comment

yd8534976 commented Dec 23, 2019 •

edited

Loading

tsawada Dec 24, 2019

yd8534976 Dec 24, 2019

tsawada Dec 24, 2019

yd8534976 Dec 24, 2019

tsawada Dec 24, 2019

tsawada left a comment

tsawada Dec 24, 2019

Use symlink for output_template/python/lmnet/utils/output.py #688

Use symlink for output_template/python/lmnet/utils/output.py #688

Conversation

yd8534976 commented Dec 18, 2019 • edited Loading

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

bo-code-review-bot bot commented Dec 20, 2019

tsawada commented Dec 23, 2019

iizukak left a comment

Choose a reason for hiding this comment

tsawada left a comment

Choose a reason for hiding this comment

yd8534976 commented Dec 23, 2019 • edited Loading

tsawada Dec 24, 2019

Choose a reason for hiding this comment

yd8534976 Dec 24, 2019

Choose a reason for hiding this comment

tsawada Dec 24, 2019

Choose a reason for hiding this comment

yd8534976 Dec 24, 2019

Choose a reason for hiding this comment

tsawada Dec 24, 2019

Choose a reason for hiding this comment

tsawada left a comment

Choose a reason for hiding this comment

tsawada Dec 24, 2019

Choose a reason for hiding this comment

yd8534976 commented Dec 18, 2019 •

edited

Loading

yd8534976 commented Dec 23, 2019 •

edited

Loading