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

Add concatenation support for multiple feature libraries #72

Merged
merged 6 commits into from
Apr 30, 2020
Merged

Add concatenation support for multiple feature libraries #72

merged 6 commits into from
Apr 30, 2020

Conversation

kopytjuk
Copy link
Contributor

@kopytjuk kopytjuk commented Apr 29, 2020

Currently, there is no straightforward way to combine multiple feature libraries into a single feature mapper.

I implemented this workflow:

from pysindy.feature_library import IdentityLibrary
from pysindy.feature_library import PolynomialLibrary
from pysindy.feature_library import ConcatLibrary

ident_lib = IdentityLibrary()
poly_lib = PolynomialLibrary()
concat_lib = ConcatLibrary([ident_lib, poly_lib]

What do you think?

@kopytjuk kopytjuk changed the title Add concatenation support for multiple libraries Add concatenation support for multiple feature libraries Apr 29, 2020
Copy link
Member

@briandesilva briandesilva left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@briandesilva
Copy link
Member

Thanks for your contribution! Thanks especially for including detailed comments and unit tests. I'm happy to merge this as is, but I'd like to give my collaborators @Ohjeah and @kpchamp a chance to give any input they might have before I do.

@Ohjeah
Copy link
Collaborator

Ohjeah commented Apr 30, 2020

Thanks for your contribution. I was about to suggest a similar feature.

What I would like to see is a more intuitive api, such as

from pysindy.feature_library import IdentityLibrary
from pysindy.feature_library import PolynomialLibrary
from pysindy.feature_library import ConcatLibrary

ident_lib = IdentityLibrary()
poly_lib = PolynomialLibrary()

my_lib = ident_lib + poly_lib

assert isinstance(my_lib, ConcatLibrary)

You'd need to implement BaseFeatureLibrary.__add__ for this to work.

@kopytjuk
Copy link
Contributor Author

kopytjuk commented Apr 30, 2020

Thanks for your contribution. I was about to suggest a similar feature.

What I would like to see is a more intuitive api, such as

from pysindy.feature_library import IdentityLibrary
from pysindy.feature_library import PolynomialLibrary
from pysindy.feature_library import ConcatLibrary

ident_lib = IdentityLibrary()
poly_lib = PolynomialLibrary()

my_lib = ident_lib + poly_lib

assert isinstance(my_lib, ConcatLibrary)

You'd need to implement BaseFeatureLibrary.__add__ for this to work.

Thanks for you review!

The only drawback of the __add__ solution is the implementation of ConcatLibrary within the feature_library.py right after the BaseFeatureLibrary class.

The reason for that is, that it is impossible to do circular imports in Python (i.e. implementing ConcatLibrary in concat_library.py is not allowed, because there we import BaseFeatureLibrary from feature_library.py. In feature_library.py we import ConcatLibrary from concat_library.py in order to use it in the __add__ method of BaseFeatureLibrary.

However, the desired workflow with + operator is implemented.

@codecov-io
Copy link

codecov-io commented Apr 30, 2020

Codecov Report

Merging #72 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   95.51%   95.67%   +0.16%     
==========================================
  Files          18       18              
  Lines         780      810      +30     
==========================================
+ Hits          745      775      +30     
  Misses         35       35              
Impacted Files Coverage Δ
pysindy/feature_library/__init__.py 100.00% <100.00%> (ø)
pysindy/feature_library/feature_library.py 100.00% <100.00%> (ø)

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 3425922...5d9cc8d. Read the comment docs.

@Ohjeah
Copy link
Collaborator

Ohjeah commented Apr 30, 2020

Thanks. Having the ConcatLibrary right next to the BaseFeatureLibrary is fine for me since they both should not be used directly.

@briandesilva briandesilva merged commit e205dea into dynamicslab:master Apr 30, 2020
@kopytjuk kopytjuk deleted the concat-libraries branch April 30, 2020 19:06
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
Add concatenation support for multiple feature libraries
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request May 9, 2024
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request May 9, 2024
Auto upload to PyPI, minor bugfixes, doc updates
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.

4 participants