Add transform objects for temporal point processes (#294) #341

canerturkmen · 2019-09-26T09:01:44Z

Issue #, if available:

This is the first PR setting up the infrastructure for temporal point processes in GluonTS (#294).

Description of changes:

Put simply it introduces the ContinuousTimeInstanceSplitter object, which somewhat replicates the logic of an InstanceSplitter for but for point processes in continuous time. I thought this was a good first PR for me, and a good place to start the process of bringing in TPPs.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

canerturkmen · 2019-09-26T09:32:17Z

I understand both test fails are related to building docs on the GPU instance. Can't tell if they're related to the PR.

lostella · 2019-09-26T09:37:09Z

Can't tell if they're related to the PR.

They aren't, we will look into the CI configuration. And thanks for the PR!

lostella · 2019-09-26T09:48:42Z

src/gluonts/transform.py

@@ -155,6 +155,42 @@ def __call__(self, ts: np.ndarray, a: int, b: int) -> np.ndarray:
 return np.array([b])


+class ContinuousTimePointSampler:


This abstraction looks very similar to the InstanceSampler: https://github.com/awslabs/gluon-ts/blob/a97518d436dbb8e2cb6e2958d4ab899e5fa43a9d/src/gluonts/transform.py#L94

The only difference seems the num_instances constructor argument here. I'm wondering if the two could be condensed somehow?

So in continuous time you sample between points in time (float), as opposed to indices (int).

codecov-io · 2019-11-18T10:02:07Z

Codecov Report

Merging #341 into master will increase coverage by 0.07%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage    83.7%   83.78%   +0.07%     
==========================================
  Files         159      159              
  Lines        9403     9458      +55     
==========================================
+ Hits         7871     7924      +53     
- Misses       1532     1534       +2

Impacted Files	Coverage Δ
src/gluonts/transform.py	`89.69% <96.42%> (+0.6%)`	⬆️

canerturkmen · 2019-11-18T10:03:08Z

@lostella Is this a good time to bump this? :)

lostella · 2019-11-18T10:15:02Z

@lostella Is this a good time to bump this? :)

Definitey :-) will go through it asap

canerturkmen · 2019-11-25T12:06:19Z

May I suggest @lovvge as a reviewer as well?

src/gluonts/transform.py

mbohlkeschneider · 2019-11-27T09:19:40Z

src/gluonts/transform.py

+ length of the interval seen before making prediction
+ future_interval_length
+ length of the interval that must be predicted
+ train_sampler


I'm confused. It says "does not accept time-series fields" above?

I'm making it a little more verbose now, but the intention was that the Transformation does not take time_series_fields (not target_field), which I remember were intended for features?

Usually the names are not defined in the transformation chain but in the estimator so it does not really matter.

src/gluonts/transform.py

mbohlkeschneider · 2019-11-27T09:46:30Z

src/gluonts/transform.py

+
+ for future_start in sampling_times:
+
+ r = dict()


Why is this necessary? You could modify your data copy (d) directly without introducing a new dict, no? This would avoid introducing a new variable and the adding the other fields later. Either way, r should be of type DataEntry if this is needed.

d was initialized outside of the loop here (this may be a spurious copy, now that I look at it). ris the output dict.

I'm not a fan of the style in InstanceSplitter where d is "copied" once per each output. However this is (probably?) a shallow copy. Moreover, features an explicit

del d[ts_field]

I thought this style was a little safer, or maybe I'm missing something here?

In any case, ditto for DataEntry 👍

The only reason I would regard this as safer is if you have to do multiple transformations that rely on one original entry on in the DataEntry (which would make sense to have it immutable in that case). I'm not a fan because it introduces more code than necessary, but tastes differ :-).

Yup. I agree. I thought in flatmap_transformation this is indeed the intention that multiple entries derive from a single input. Hence the extra code, to stress that the input is immutable.

Just crossing the t's here to make sure :)

src/gluonts/transform.py

lostella reviewed Sep 26, 2019

View reviewed changes

timoschowski added this to the v0.4 milestone Oct 9, 2019

jaheba modified the milestones: v0.4, v0.5 Oct 24, 2019

canerturkmen force-pushed the rmtpp_transform branch from 461d8ee to a21681d Compare November 18, 2019 09:47

lostella requested a review from lovvge November 25, 2019 20:24

lovvge reviewed Nov 25, 2019

View reviewed changes

src/gluonts/transform.py Outdated Show resolved Hide resolved

canerturkmen force-pushed the rmtpp_transform branch 2 times, most recently from 84da906 to 5bb91fc Compare November 25, 2019 21:51

mbohlkeschneider requested changes Nov 27, 2019

View reviewed changes

mbohlkeschneider mentioned this pull request Nov 27, 2019

Refactoring transforms #481

Closed

canerturkmen pushed a commit to canerturkmen/gluon-ts that referenced this pull request Dec 6, 2019

TPP transform revisions (awslabs#341)

442f7ff

canerturkmen force-pushed the rmtpp_transform branch from 5bb91fc to 442f7ff Compare December 6, 2019 16:49

canerturkmen pushed a commit to canerturkmen/gluon-ts that referenced this pull request Dec 6, 2019

TPP transform revisions (awslabs#341)

a169110

canerturkmen force-pushed the rmtpp_transform branch from 442f7ff to a169110 Compare December 6, 2019 16:53

mbohlkeschneider previously approved these changes Dec 6, 2019

View reviewed changes

add transform objects for temporal point processes (awslabs#294)

8bb133a

canerturkmen dismissed mbohlkeschneider’s stale review via 8bb133a December 6, 2019 22:13

canerturkmen force-pushed the rmtpp_transform branch from a169110 to 8bb133a Compare December 6, 2019 22:13

mbohlkeschneider approved these changes Dec 7, 2019

View reviewed changes

canerturkmen merged commit fbcb244 into awslabs:master Dec 7, 2019

canerturkmen deleted the rmtpp_transform branch December 7, 2019 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transform objects for temporal point processes (#294) #341

Add transform objects for temporal point processes (#294) #341

canerturkmen commented Sep 26, 2019

canerturkmen commented Sep 26, 2019

lostella commented Sep 26, 2019

lostella Sep 26, 2019

canerturkmen Sep 26, 2019

codecov-io commented Nov 18, 2019 •

edited

Loading

canerturkmen commented Nov 18, 2019

lostella commented Nov 18, 2019

canerturkmen commented Nov 25, 2019

mbohlkeschneider Nov 27, 2019

canerturkmen Dec 6, 2019

mbohlkeschneider Dec 6, 2019

mbohlkeschneider Nov 27, 2019

canerturkmen Dec 6, 2019

mbohlkeschneider Dec 6, 2019

canerturkmen Dec 6, 2019

		@@ -155,6 +155,42 @@ def __call__(self, ts: np.ndarray, a: int, b: int) -> np.ndarray:
		return np.array([b])


		class ContinuousTimePointSampler:

Add transform objects for temporal point processes (#294) #341

Add transform objects for temporal point processes (#294) #341

Conversation

canerturkmen commented Sep 26, 2019

canerturkmen commented Sep 26, 2019

lostella commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 18, 2019 • edited Loading

Codecov Report

canerturkmen commented Nov 18, 2019

lostella commented Nov 18, 2019

canerturkmen commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 18, 2019 •

edited

Loading