Second round of updates #85
Conversation
…be segment; one step closer to eliminating scipy
Pull Request Test Coverage Report for Build 8707470054Details
💛 - Coveralls |
| @@ -492,7 +492,7 @@ class Interpolator1D(InterpolatorBase): | |||
|
|
|||
| def __init__(self, x_data: np.ndarray, y_data: np.ndarray | list[float]): | |||
| """ | |||
| 1D Interpolation Class, currently a wrapper for scipy interpolator, but soon just a simple interpolator | |||
| 1D Interpolation Class, currently a wrapper for sci py interpolator, but soon just a simple interpolator | |||
There was a problem hiding this comment.
Yeah, I didn't want to add scipy to my spelling list, so just split the word. Hopefully soon we won't have to mention it at all...
| @@ -1,36 +1,36 @@ | |||
| import os | |||
There was a problem hiding this comment.
There are a lot of places where os isn't need anymore as I've moved to using pathlib.
| self.interp_g = Interpolator1DFromFile(path_g) | ||
| p = Path(inputs['g-function-path']) | ||
| if not p.is_absolute(): | ||
| p = Path.cwd() / inputs['g-function-path'] |
There was a problem hiding this comment.
This pattern is in the code pretty much anywhere we are reading a path from the input dict. I wonder if there is a better way to do it.
And after taking a quick look, it looks like the Path's resolve() method will handle this. If it's relative, it will apply cwd() to make it absolute. And if it's absolute it will leave it. And it will walk through links and stuff. I'll fix that up.
TIL.
There was a problem hiding this comment.
Yeah, I try to resolve() any pathlib object at instantiation, and then it all just works. This was all developed before I was a pathlib convert. 🙏
| elif 'lntts' and 'g-values' in inputs: | ||
| data_g = np.transpose(np.array([inputs['lntts'], inputs['g-values']])) | ||
| self.interp_g = Interpolator1D(data_g[:, 0], data_g[:, 1]) | ||
| self.interp_g: InterpolatorBase = Interpolator1D(data_g[:, 0], data_g[:, 1]) |
There was a problem hiding this comment.
Mimicking some polymorphism here by trying to convince the IDE to treat this as a base class. No functional effect for Python itself...just trying to help with autocompletion.
| @@ -23,16 +23,14 @@ def __init__(self, inputs: dict): | |||
| self.sub_hr = SubHour(inputs) | |||
|
|
|||
| # set expansion rate. apply default if needed. | |||
| try: | |||
| self.exp_rate = 1.62 | |||
There was a problem hiding this comment.
Minor change, which I think should get rid of a few pragma: no covers
| @@ -111,22 +105,20 @@ def calc_temporal_superposition(self, time_step: int, flow_rate: float = None) - | |||
| if not flow_rate: | |||
| g_b = self.interp_g_b.interpolate(lntts) | |||
| else: | |||
| g_b = np.flipud(self.interp_g_b(lntts, flow_rate)) | |||
| # TODO: This is not covered by tests, and may need adjusting the interpolator class to work properly | |||
| g_b = np.flipud(self.interp_g_b.interpolate(lntts, flow_rate)) | |||
There was a problem hiding this comment.
Still leaving this for now. I think the best thing would be to work up a unit test that exercises it, see it fail, and then fix it :)
| from math import pi | ||
|
|
||
| import numpy as np | ||
| from scipy.integrate import solve_ivp | ||
| from scipy.integrate import RK45 |
There was a problem hiding this comment.
As a step forward, I am telling scipy to explicitly use the RK45 stepper. This should make sure we have a near identical set of results on the unit tests when we use our own RK45 routines.
| ######### KEEP THE LINE SAYING "OPTIONAL" IN PLACE HERE ######### | ||
| ######### IT IS USED BY SETUP.PY WHEN BUILDING PACKAGES ######### | ||
|
|
||
| # OPTIONAL # |
There was a problem hiding this comment.
Added a line in the requirements file that splits the required install deps from the optional deps. In a future step, we could consider splitting the requirements into multiple files and just setting up dependencies between the files. Or maybe this is easier solved with pyproject.toml. breathe
| 'glhe.standalone', | ||
| 'glhe.topology', | ||
| 'glhe.utilities', | ||
| ], |
There was a problem hiding this comment.
Could use a finder, but it's fine either way.
| license='MIT', | ||
| install_requires=requirements, | ||
| long_description=open('README.md').read(), | ||
| long_description_content_type='text/markdown', | ||
| entry_points={'console_scripts': ['glhe=glhe.standalone.plant_loop:main',]} |
There was a problem hiding this comment.
Not sure if this entry point should be named differently or anything, it's just a start.
| temp_dir = tempfile.mkdtemp() | ||
| temp_file = os.path.join(temp_dir, 'temp.json') | ||
| temp_dir = Path(tempfile.mkdtemp()) | ||
| temp_file = temp_dir / 'temp.json' |
There was a problem hiding this comment.
There were quite a few places that things like this were repeated. We could add a unit test fixture to collect a bunch of these. It would be nice to move the unit_tests folder inside the glhe code, that way the fixture import would look like from glhe.tests.fixture import GLHEFixture, rather than from unit_tests ... like it would be right now.
I'll try to itemize changes here. I still want to eliminate the scipy dependency in the u-tube segment class before wrapping up here. That will move scipy to only the functions class. And I might even pull it into its own file as well.