Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[clojure] clojurify function names in image.clj namespace #15121

Merged
merged 5 commits into from
Jun 4, 2019

Conversation

Chouffe
Copy link
Contributor

@Chouffe Chouffe commented Jun 1, 2019

Description

The image namespace is using some functions that are not named the clojure way. It is an attempt to clojurify these functions.
The namespaces are supposed to be imported qualified and having the suffix -image in the following functions is redundant

  • resize-image
  • decode-image
  • read-image

One should be able to call image/resize, image/decode, image/read instead.

The function to-image is misleading as we dont know what is turned into an image. I suggest to name it ndarray->image instead.

  • deprecated is added in metadata
  • deprecated is added in docstrings

@Chouffe Chouffe requested a review from gigasquid as a code owner June 1, 2019 18:56
Copy link
Contributor

@kedarbellare kedarbellare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! let me merge this.

as an aside, are there other places in examples where *-image functions are used? if yes, can you replace those usages as well?

(defn decode
"Decodes an image from an input stream with OpenCV.
`input-stream`: `InputStream` - Contains the binary encoded image
`color-flag`: 0 or 1 - Convert decoded image to grayscale (0) or color (1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing i'm wondering is whether color-flag should be specified as :grayscale or :color and we handle the conversion to 0, 1 in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point! Let me fix this then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to use another name for the keyword :color instead of :color-flag. I am open to better name for that. The problem is that it is not possible to have two different specs with the same name.

@kedarbellare
Copy link
Contributor

@Chouffe : you need to fix some conflicts because i merged your other PR

@Chouffe
Copy link
Contributor Author

Chouffe commented Jun 2, 2019

lgtm! let me merge this.

as an aside, are there other places in examples where *-image functions are used? if yes, can you replace those usages as well?

Will do! Thanks for reviewing so promptly @kedarbellare
I could not find any files in examples where *-image functions are used.

@Chouffe Chouffe force-pushed the chouffe/clojure-image-clojury branch from 4947d03 to cc54869 Compare June 2, 2019 19:40
@Chouffe Chouffe force-pushed the chouffe/clojure-image-clojury branch from 1969463 to c665fdc Compare June 2, 2019 19:45
@Chouffe
Copy link
Contributor Author

Chouffe commented Jun 3, 2019

Should be ready to 🚢 now @kedarbellare

@kedarbellare kedarbellare merged commit 28c528e into apache:master Jun 4, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* [clojure] clojurify function names in image.clj namespace

* move deprecated to the proper location for defn

* rename color-flag to color and use :color :grayscale as values

* add rm dest-path in with-file

* change `color-flag` to `color` in `color->int`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants