-
Notifications
You must be signed in to change notification settings - Fork 653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Extension] Add GluonTS extension #1903
Conversation
extensions/gluonTS/src/main/java/ai/djl/gluonTS/SampleForeCast.java
Outdated
Show resolved
Hide resolved
* | ||
* @return the String converted to lower case. | ||
*/ | ||
public String lowerCase() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't seem useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will delete it, it came from my first version
@@ -0,0 +1,30 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need log4j at lib level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
@@ -0,0 +1,81 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move examples into top level example folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
@@ -0,0 +1,17 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check in those class when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid use git merge in future, it's hard to read history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I suggest you break your changes into smaller pieces if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that, I wondering is there any tutorial or cmd for me to split the changes into pieces? Cuz, im not familiar with git
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that, I wondering is there any tutorial or cmd for me to split the changes into pieces? Cuz, im not familiar with git
Since everything is in your local and they are new files, you can create a new branch and manually copy these files and create a new PR with selected files. You can create multiple branches, each branch for a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I got it.
…nown error occur. fix this using manager.create(new Shape(0))
9018e56
to
75a6d36
Compare
…nction, remove log4j, move examples Co-authored-by: Frank Liu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example still failing.
Please make sure output NDArrays are created with the same NDManager as input NDArray.
@Override | ||
public NDList processInput(TranslatorContext ctx, TimeSeriesData input) { | ||
NDManager manager = ctx.getNDManager(); | ||
this.startTime = input.getStartTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use member variable startTime
is not thread-safe.
You can pass input to processOutput() by set attachment.
ctx.setAttachment("input", input);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because forecast in the example needs to access the model's predictor's output outside of the translator's ctx lifecycle, I new an NDManager in the constructor of the SampleForecast to create an NDArray to ensure that the result is not released. But this would make the NDArray lifecycle in SampleForecast inconvenient to manage, and I don't have a good solution for that. One idea is to replace NDArray with float[] in SampleForecast, but some of the statistical operations would be inconvenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Carkham To clarify what you mentioned here, did you mean that you need to access the model predictor's output, which should not be released outside the translator's ctx?
Codecov Report
@@ Coverage Diff @@
## master #1903 +/- ##
============================================
- Coverage 72.08% 69.97% -2.12%
- Complexity 5126 5791 +665
============================================
Files 473 569 +96
Lines 21970 25708 +3738
Branches 2351 2777 +426
============================================
+ Hits 15838 17988 +2150
- Misses 4925 6356 +1431
- Partials 1207 1364 +157
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Before modifying NDManager in this line, my local test can pass this example, but it fails after modification, and an error is reported as "Native resource has been release already". So I set the NDManager of SampleForecast.sample to the parent NDManager of the model output which actually is the NDManager of predicotr and it works well. In this following example code segment:
|
Change-Id: I3df5e17e427a0f21a1ded862d72e7c2a5ce894c7
It looks like this error is not triggered in the PR test. If the error happens after the modification, would it be detected in the PR test? |
* init commit of GluonTS, and the transform is organised by static method * debug only: add transformer example * feature: DeepARExample and model files * bug: when the start index >= end index in slice of the tsData, an unknown error occur. fix this using manager.create(new Shape(0)) * feature: extract interface from the static method in transform * fromat Java and checkStyle * review comments: rename module, delete default constructor, delete function, remove log4j, move examples Co-authored-by: Frank Liu <[email protected]> * delete the static NDManager and format the code. * Refactor classes in timeseries package * bug fix: add the batch_size dim for the Shape(0) data in the split error case * Rename ForeCast to Forecast. Add the description of module structure in README.md. * Replace the thread unsafe usage with ctx.getAttachment * Make predictor.manager to be the NDManager of SampleForecast.samples * Refactor timeseries API Change-Id: I3df5e17e427a0f21a1ded862d72e7c2a5ce894c7 Co-authored-by: Frank Liu <[email protected]>
Description
We create GluonTS extension based on djl.
It contains three parts including: data entry, transform, translator.
In the data entry section, we provide efficient key-value pairs for data preprocessing and conversion into
NDList
in GluonTS datasets and translators. And in the inference task, we also designed aForecast
class similar to GluonTS, and provided related query methods.In the transform part, the corresponding data pre-processing process is provided through
GluonTSData
, and the easy-to-use interface and static methods are provided to complete this process.Among them, convert is responsible for the shape transformation of
NDArray
, feature is responsible for generating time series features, field is responsible for field modification, and split is responsible for sampling time series data in different time periods during inference and train.In the translator section, we provide translators for two commonly used GluonTS time series models. Most of the content of these two Translators are similar, only some parameters have been modified. Please note that users can quickly build their own GluonTS model Translator according to the transform we provide.
At the same time, we also added two examples of inference with the GluonTS model in the examples.
See README.md for detailed description.