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

Feature/pca based cost #32

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

statefb
Copy link

@statefb statefb commented May 23, 2020

Hi, implemented #29
Main contributions are followings:

  • PCA-based cost functions
    • Q statistic (reconstruction error of PCA)
    • Hotelling's T2 statistic
  • Data generation for demo
    • Added noise generation function to datasets.pw_normal

TODO:

  • arrange docstring

Any feedback + code review would be appreciated.

@statefb statefb marked this pull request as draft May 23, 2020 15:31
@statefb statefb marked this pull request as ready for review May 23, 2020 15:31
@statefb statefb marked this pull request as draft May 23, 2020 15:35
@statefb statefb marked this pull request as ready for review May 23, 2020 15:35
@deepcharles
Copy link
Owner

Many thanks.
I will take a look this week.

@deepcharles
Copy link
Owner

sorry for the delay, I'm starting to go through your PR

@deepcharles
Copy link
Owner

Also can you format your code (I personnaly use flake8). Note that I do not mind long lines (for now).

image

For now, it is not well enforced in the rest of the package but it will be soon, so I'd rather save me some work now.

@statefb
Copy link
Author

statefb commented Jun 16, 2020

Also can you format your code (I personnaly use flake8).

I forgot to format my code before PR. Sorry for omitting the very basic things. I'll add some commit later.

@deepcharles
Copy link
Owner

Can you resend me the invite link to your fork? I'd like to correct some minor things.

@statefb
Copy link
Author

statefb commented Jun 24, 2020

Can you resend me the invite link to your fork? I'd like to correct some minor things.

I resent invitation.
I'll work on pep8 on this weekend.

@statefb
Copy link
Author

statefb commented Jun 28, 2020

I formatted scripts based on flake8 except for long line. If you have another request to modify code, please let me know.

@deepcharles
Copy link
Owner

I have added several commits to your PR. However the documentation is not clear enough:

  • the equation of cost functions do not seem right, can you clarify them.
  • the n_component argument is not explained.
  • make the correct citation to the related article (I have done it in costTSquared).

@statefb
Copy link
Author

statefb commented Aug 5, 2020

Thank you for reviewing and adding commits.
I fixed some codes to add n_component explanation and citation to CostQResiduals in the same way as TSquared.

Could you explain about the detail why the equation of cost functions seem not right?
It's true that the equations are different from the codes, but the results are the same. For example, in the case of CostQResiduals, U_p^T * U_p * y_t (where U_p is singular vectors) represents reconstructed value of y_t using PCA, which can be implemented by PCA module of sklearn package as abstracted and readable style.
Should I implement using numpy module as plain linear algebra style?

@deepcharles
Copy link
Owner

Could you explain about the detail why the equation of cost functions seem not right?

You are right, the equation is ok. I thought that $\bar{y}_t$ referred to the mean value on the segment while it only referred to the transformed value, I will slightly change the notation to remove any misunderstanding.

Also, I would like to add a usage example (in the doc) for those cost functions, so that it can be easily understood. Do you have one at hand? I am not able to replicate the examples in the article (see Section 4: Motivating examples). If you could share (directly in this discussion) a few lines of code to simulate the signals of the article, that would be great.

On a side note, from now on, I will add the names of the contributors in the doc, is your github handle (statefb) ok?

@statefb
Copy link
Author

statefb commented Sep 6, 2020

Also, I would like to add a usage example (in the doc) for those cost functions, so that it can be easily understood. Do you have one at hand?

Unfortunately, I don't have any codes to replicate the "dynamic" example. Actually I wanted to cite a paper which only focus on non-dynamic PCA based segmentation, but I haven't found one yet for now. As seen in example in the current doc, my PR just for non-dynamic one (it's easy to expand and apply to dynamic system, though).
Could you accept to send another PR when I write dynamic example?

On a side note, from now on, I will add the names of the contributors in the doc, is your github handle (statefb) ok?

Yes, thank you.

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.

2 participants