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

[MXNET-1287] Feat dep #14668

Merged
merged 3 commits into from
Apr 19, 2019
Merged

[MXNET-1287] Feat dep #14668

merged 3 commits into from
Apr 19, 2019

Conversation

zachgk
Copy link
Contributor

@zachgk zachgk commented Apr 10, 2019

Description

This PR fixes the feature/deprecation warnings and makes them display as part of the Maven run by default. Most of the fixes are related to the change to IndexedSeq[DataDesc]. Contains a part of #13995.

@lanking520 @andrewfayres

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@zachgk
Copy link
Contributor Author

zachgk commented Apr 10, 2019

@mxnet-label-bot add [Scala, Maven, pr-awaiting-review]

Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

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

  1. General question: why we need to import postfixop library?
  2. Please also explain the comments below

All the rest looks fine since covered by the tests.

}
}

private val _provideLabel: ListMap[String, Shape] = {
@deprecated("Please use provideDataDesc instead", "1.3.0")
override def provideLabel: ListMap[String, Shape] = {
Copy link
Member

Choose a reason for hiding this comment

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

Why not make it private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _provideLabel was a private implementation of the externally accessible provideLabel. I thought there was no point in having both so I combined them and left the public one.

@@ -39,7 +39,7 @@ object NeuralStyle {
private val logger = LoggerFactory.getLogger(classOf[NeuralStyle])

def preprocessContentImage(path: String, longEdge: Int, ctx: Context): NDArray = {
val img = Image(new File(path))
val img = Image.fromFile(new File(path))
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a warning against using Image(). I don't remember what the warning was, though.

Copy link
Contributor Author

@zachgk zachgk left a comment

Choose a reason for hiding this comment

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

The import of postfix, along with the rest of scala.language, is to enable various language features that are not default. The postfix operation seemed to mostly be used for CLI execution such as:

"python -m SimpleHTTPServer" !

}
}

private val _provideLabel: ListMap[String, Shape] = {
@deprecated("Please use provideDataDesc instead", "1.3.0")
override def provideLabel: ListMap[String, Shape] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _provideLabel was a private implementation of the externally accessible provideLabel. I thought there was no point in having both so I combined them and left the public one.

@@ -39,7 +39,7 @@ object NeuralStyle {
private val logger = LoggerFactory.getLogger(classOf[NeuralStyle])

def preprocessContentImage(path: String, longEdge: Int, ctx: Context): NDArray = {
val img = Image(new File(path))
val img = Image.fromFile(new File(path))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a warning against using Image(). I don't remember what the warning was, though.

@lanking520
Copy link
Member

Does the Scala GPU failure related to your changes?

@lanking520 lanking520 merged commit 5b6e25b into apache:master Apr 19, 2019
kedarbellare pushed a commit to kedarbellare/incubator-mxnet that referenced this pull request Apr 20, 2019
* Feature and Deprecation Warnings

* Address feature and deprecation Warnings (mostly provideData)

* Fix name search
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Feature and Deprecation Warnings

* Address feature and deprecation Warnings (mostly provideData)

* Fix name search
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Maven pr-awaiting-review PR is waiting for code review Scala
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants