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

update fpca references #636

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

aleexarias
Copy link

Resolves #634

The reference in _fpca is moved into the class docstring instead of the private function it was on, using footcite/footbibliography

  • I have performed a self-review of my code
  • The code conforms to the style used in this package
  • The code is fully documented and typed (type-checked with Mypy)
  • I have added thorough tests for the new/changed functionality

Copy link
Member

@vnmabus vnmabus left a comment

Choose a reason for hiding this comment

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

Some changes requested.

skfda/preprocessing/dim_reduction/_fpca.py Outdated Show resolved Hide resolved
skfda/preprocessing/dim_reduction/_fpca.py Show resolved Hide resolved
@@ -25,7 +25,22 @@
# to explore datasets and obtain conclusions about said dataset using this
# technique.
#
# First we are going to fetch the Berkeley Growth Study data. This dataset
# First we will introduce what FPCA is and why it is commonly used in many
Copy link
Member

Choose a reason for hiding this comment

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

I would just introduce it, without this line. I think it is safe to assume that the reader already knows what FDA is, at least by skimming the readme and/or the tutorial (otherwise we would need to add such an introduction to every example).

# studies. Generally, functional data is represented as functions X(t) where
# t might be time or some other continuous variable. In theory, these
# functions can have an infinite number of data points (they can be treated as
# infinite dimension vector spaces) so the goal of FPCA is to reduce the
Copy link
Member

Choose a reason for hiding this comment

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

I would expect some definition ("FPCA is ..."), like "FPCA is a dimensionality reduction method for functional data. The goal of FPCA is to reduce [...]".

# variation across the function (the most important directions in which the
# curves vary). FPCA can be though of as a basis expansion, but what
# distinguishes FPCA is that among all basis expansions that use K components
# for a fixed K, the FPC expansion explains most of the variation in X.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# for a fixed K, the FPC expansion explains most of the variation in X.
# for a fixed K, the FPCA expansion explains most of the variation in X.

# For more information abour FPCA and its objectives, see
# :footcite:ts:`wang+chiou+muller_2016_fpca`.
#
# In second place, we are going to fetch the Berkeley Growth Study data. This dataset
Copy link
Member

Choose a reason for hiding this comment

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

This is really the first step of the tutorial, the previous paragraph being a short explanation of the method. I would drop that commit.

# principal components, which are the directions that capture the main modes
# of variation across the function (the most important directions in which the
# curves vary). FPCA can be though of as particular case of a basis expansion
# that uses K components for a fixed K, with the particularity being that the
Copy link
Member

Choose a reason for hiding this comment

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

I actually liked very much your sentence "among all basis expansions that use K components for a fixed K, the FPC expansion explains most of the variation in X.". Why did you remove it?

I also noted that your commit messages are not very informative. If you want, you may try to use git rebase in interactive mode to rewrite your git history and squash all your commits together (or in other situations, to squash some of them, change the messages or reorder them). It is not strictly necessary (I could squash them when merging, or leave them) but it is a tool that is sometimes useful and you may want to try.

Note that, in general, rewriting git history should only be done in private branches that you share with nobody, or with a few other developers you communicate with (so, never rebase master or develop). After rebasing you necessarily have to push with the "--force" (or better "--force-with-lease") option, which means that any other developer that is using the branch would have it in an incompatible state and should reset it.

@aleexarias aleexarias force-pushed the fpca_references_fixed branch 2 times, most recently from 2026b93 to 35bf3bf Compare October 25, 2024 10:46
allcontributors bot and others added 3 commits October 25, 2024 20:05
update fpca references

FPCA reference added to docstring + added FPCA explanation in example

Fix references in FPCA
examples/plot_fpca.py Outdated Show resolved Hide resolved
# for a fixed K, the FPCA expansion explains most of the variation in X.
#
# For more information abour FPCA and its objectives, see
# :footcite:ts:`wang+chiou+muller_2016_fpca`.
Copy link
Member

Choose a reason for hiding this comment

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

There is no footbibliography, so the references are not shown in this file.

when fitting a FDataBasis or FDataGrid, except for ``components_basis``.

For more information about the implementation of the computation of the
first principal components see :footcite:ts:`silverman_2005_basisfuncexp`.
Copy link
Member

Choose a reason for hiding this comment

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

The reference is wrong. Please, check the generated documentation (either locally, or in the PR, in the details link of the Readthedocs check).

Co-authored-by: Carlos Ramos Carreño <[email protected]>

New commit
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.83%. Comparing base (2f48baa) to head (68cb73e).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #636   +/-   ##
========================================
  Coverage    86.83%   86.83%           
========================================
  Files          157      157           
  Lines        13522    13522           
========================================
  Hits         11742    11742           
  Misses        1780     1780           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Fix references in FPCA
2 participants