Skip to content
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

Allow the user to disable scaling on GASF and GADF images #18

Merged
merged 3 commits into from Feb 9, 2019

Conversation

TobCar
Copy link
Contributor

@TobCar TobCar commented Feb 5, 2019

The normalization step when generating the compound images is really useful. However, if the time series data being turned into a compound image is an observation of a longer series of data with a larger range of values, the normalization removes valuable information.

I'm part of a research team at Queen's University and we are using your library to predict who will experience delirium in the ICU. While developing our model we realized the normalization step made it impossible for the model to learn. To give a practical example, one of the features we have is a patient's heart rate. One patient may have a peak heart rate of 200 whereas another one may have a peak heart rate of 140. The normalization treats both peak values as 1 because the smaller values belongs to a different patient and a different training case. To resolve this we, normalize the data across all the patients before creating the compound images without the normalization step. This way, the GASF and GADF have values within a limited range as required, but we keep information regarding what observations are greater than others to keep the observation in context.

@pep8speaks
Copy link

pep8speaks commented Feb 5, 2019

Hello @TobCar! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 05, 2019 at 15:36 Hours UTC

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #18 into master will decrease coverage by 0.18%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
- Coverage   82.14%   81.96%   -0.19%     
==========================================
  Files          27       27              
  Lines        1759     1763       +4     
  Branches      324      326       +2     
==========================================
  Hits         1445     1445              
- Misses        163      165       +2     
- Partials      151      153       +2
Impacted Files Coverage Δ
pyts/image/image.py 70.23% <33.33%> (-1.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e1e51...631f118. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #18 into master will decrease coverage by 0.18%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
- Coverage   82.14%   81.96%   -0.19%     
==========================================
  Files          27       27              
  Lines        1759     1763       +4     
  Branches      324      326       +2     
==========================================
  Hits         1445     1445              
- Misses        163      165       +2     
- Partials      151      153       +2
Impacted Files Coverage Δ
pyts/image/image.py 70.23% <33.33%> (-1.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e1e51...8f87267. Read the comment docs.

raise ValueError if 'scale=None' and if the values of X are not in [-1, 1] for GASF and GADF
@johannfaouzi
Copy link
Owner

Thank you for the PR. I did not think about the practical case when the observations are only subseries of time series.

I added a small gotcha to your PR to ensure that all the values are between -1 and 1 when the scaling is disabled.

@TobCar
Copy link
Contributor Author

TobCar commented Feb 5, 2019

Ah, that's a good catch!

@TobCar
Copy link
Contributor Author

TobCar commented Feb 9, 2019

Hi Johann would it be possible to merge the PR?

@johannfaouzi johannfaouzi merged commit b4167ad into johannfaouzi:master Feb 9, 2019
@johannfaouzi
Copy link
Owner

Sure, I totally forgot about it. Sorry for that!

@TobCar
Copy link
Contributor Author

TobCar commented Feb 9, 2019

Thanks again Johann! Is there a chance you could also mark a new release (0.72?), we want to cite the code as it is right now.

@johannfaouzi
Copy link
Owner

It's done! The version will be 0.7.3 (I wrote a typo when I edited your PR and I did not run the tests before uploading the new version, so I had to upload a new version once more). When you import the package and run pyts.__version__, it will output 0.7.0 because I also forgot to change the version in the __init__.py, but it does not change anything.

By the way I have worked the past few months to improve the package. This work is only local for the moment, but I hope to release the 0.8.0 version by the end of March.

@TobCar
Copy link
Contributor Author

TobCar commented Feb 9, 2019

Sounds good, I'll keep an eye out for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants