feat: add set_extrapolation_info function in morph.Morph
#255
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 23 23
Lines 1355 1355
=======================================
Hits 1354 1354
Misses 1 1
🚀 New features to boost your workflow:
|
| extrap_index_low = morph.extrap_index_low | ||
| extrap_index_high = morph.extrap_index_high | ||
|
|
||
| extrap_low = np.where(x_morph < min(x_squeezed))[0] |
There was a problem hiding this comment.
The code to compute extrap_index_*_expected and extrap_index_*_actual is identical, so we can remove the check and keep only one of them. In this case, extrap_index_*_actual is removed, which makes the implementation in the main program simpler.
There was a problem hiding this comment.
Is there an argument to be made that this work could be done in checkExtrapolation()?
|
@sbillinge @Sparks29032, it's ready for review. Please check if this is okay, and then I will code the warnings for stretching and write the tests. |
src/diffpy/morph/morph_io.py
Outdated
| cutoff_low = extrapolation_info["cutoff_low"] | ||
| cutoff_high = extrapolation_info["cutoff_high"] | ||
|
|
||
| if is_extrap_low or is_extrap_high: |
There was a problem hiding this comment.
This is getting better, but I still think it would be more readable with something like:
if low and high:
low|high flow
elif low:
low flow
elif high:
high flow
src/diffpy/morph/morphs/morph.py
Outdated
| return rv | ||
|
|
||
| def checkExtrapolation(self, x_true, x_extrapolate): | ||
| import numpy |
There was a problem hiding this comment.
is there a reason we are importing numpy locally? Put it at the top of the file?
There was a problem hiding this comment.
No. I will put it at the top of the file.
src/diffpy/morph/morphs/morph.py
Outdated
| ylabel(self.youtlabel) | ||
| return rv | ||
|
|
||
| def checkExtrapolation(self, x_true, x_extrapolate): |
There was a problem hiding this comment.
make this a private function if it is only used within this module, or give it a docstring.
Also, just to chec before we put this in the base class, is this method called in multiple sub-classes?
Finally, the function name seems to be a bit off. It seems as if it is a setter, not checking anything. Something like set_extrapolation_info() or something like that.
There was a problem hiding this comment.
The method will be called in morphsqueeze, morphshift, and morphstretch. I will add its docstring.
Yes, set_extrapolation_info will be better.
| extrap_index_low = morph.extrap_index_low | ||
| extrap_index_high = morph.extrap_index_high | ||
|
|
||
| extrap_low = np.where(x_morph < min(x_squeezed))[0] |
There was a problem hiding this comment.
Is there an argument to be made that this work could be done in checkExtrapolation()?
Yes. We can add this to that function. |
src/diffpy/morph/morph_io.py
Outdated
| if is_extrap_low or is_extrap_high: | ||
| if not is_extrap_high: | ||
| wmsg = ( | ||
| "Warning: points with grid value below " |
There was a problem hiding this comment.
Change "will be extrapolated" to "are extrapolated"
ycexiao
changed the title
checkExtrapolation function in morph.Morphset_extrapolation_info function in morph.Morph
| cutoff_low = extrapolation_info["cutoff_low"] | ||
| cutoff_high = extrapolation_info["cutoff_high"] | ||
|
|
||
| if is_extrap_low and is_extrap_high: |
There was a problem hiding this comment.
Logic flow changed according to @sbillinge's comment.
| """ | ||
|
|
||
| """Morph -- base class for defining a morph.""" | ||
| import numpy |
There was a problem hiding this comment.
import is moved to the top.
| return rv | ||
|
|
||
| def set_extrapolation_info(self, x_true, x_extrapolate): | ||
| """Set extrapolation information of the concerned morphing |
There was a problem hiding this comment.
docstring is added.
| extrapolation_info = { | ||
| "is_extrap_low": is_extrap_low, | ||
| "cutoff_low": cutoff_low, | ||
| "extrap_index_low": extrap_index_low, |
There was a problem hiding this comment.
extrap_index_* is added.
| "cutoff_high": cutoff_high, | ||
| "extrap_index_high": extrap_index_high, | ||
| } | ||
| self.extrapolation_info = extrapolation_info |
There was a problem hiding this comment.
The way the function works has changed from returning a dict to setting the object's attributes since now the name starts with set.
| extrap_index_low_expected = extrap_low[-1] if extrap_low.size else 0 | ||
| extrap_index_high_expected = extrap_high[0] if extrap_high.size else -1 | ||
|
|
||
| extrapolation_info = morph.extrapolation_info |
There was a problem hiding this comment.
Getting the index now can be done in the morph.
| assert np.allclose(y_target_actual, y_target) | ||
| assert np.allclose(x_target_actual, x_target_expected) | ||
| assert np.allclose(y_target_actual, y_target_expected) | ||
| assert extrap_index_low_actual == extrap_index_low_expected |
There was a problem hiding this comment.
extrap_index_* assertions. are added
| lambda x: ( | ||
| "Warning: points with grid value below " | ||
| f"{x[0]} will be extrapolated." | ||
| f"{x[0]} are extrapolated." |
There was a problem hiding this comment.
"will be" -> "are"
|
Nicely done, thanks @ycexiao ! |
3 participants
What problem does this PR address?
address issues mentioned in #243.
Refactor the code to throw extrapolation warnings and throw an extrapolation warning for
morph_shift.What should the reviewer(s) do?
In this PR,
checkExtrapolationis implemented atmorph.Morphto make it accessible to all derived classes.checkExtrapolation's output.