-
Notifications
You must be signed in to change notification settings - Fork 25
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
Rewrite interpolation structure to utilize extendible class structure #210
Conversation
Codecov ReportBase: 90.71% // Head: 92.43% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #210 +/- ##
==========================================
+ Coverage 90.71% 92.43% +1.72%
==========================================
Files 42 49 +7
Lines 1723 1996 +273
==========================================
+ Hits 1563 1845 +282
+ Misses 160 151 -9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
8f0819c
to
afaf775
Compare
c46841e
to
a84ca37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, some comments inline.
Due to the changes, the top-level API (the functions in interpolate.py
) now seem all untested, which is not good, that should be fixed.
a84ca37
to
4c55923
Compare
8c0c573
to
11ec002
Compare
11ec002
to
062e3ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I don't quite like yet is the structure. Having submodules interpolation
and interpolators
right next to each other is confusing.
I'd create a single submodule interpolation
(directory) and move the interpolation.py
to something like interpolation/interpolate_irfs.py
and then import those and the other classes in interpolation/__init__.py
, that way, the imports also stay backwards compatible with what is currently in interpolation.py
.
docs/interpolation.rst
Outdated
of interpolation algorithms as interpolators as well as scripts applying those to | ||
IRF components. | ||
conditions to a new IRF. Implementations of interpolation algorithms exist as interpolator | ||
objects and are applied by top-level scripts to IRF components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
objects → classes, scripts → functions?
Simple wrapper around scipy.interpolate.griddata to interpolate parametrized quantities | ||
""" | ||
|
||
import numpy as np |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import
While the current structure of
pyirf
s interpolation functionalities might suffice with only few interpolation algorithms implemented, extending it with further algorithms (e.g. Moment-Morphing and extrapolation) would result in at least some I/O-Code doubling. This PR refactors the current interpolation algorithms into a class-structure with the upside of having: