Potential API for PyGFunction #4

vtnate · 2024-11-01T22:32:01Z

This is an example showing how to pass purely data (floats) to PyGFunction and have it return a gfunction in list form.

There are lots of commented lines in this first draft, while we explore various techniques for achieving a simpler use of PyGFunction in GHED.

pygfunction/api.py

mitchute · 2024-11-08T03:35:18Z

@Myoldmopar can you take a look here? IMO this is headed in the right direction. We may want to see about some more helper methods for creating the lists of borehole params, or maybe we just leave that to the user?

pygfunction/api.py

tests/test_api.py

mitchute · 2024-11-08T15:42:36Z

pygfunction/api.py

+        for borehole_config_items in borehole_config_list:
+            _borehole = Borehole(*borehole_config_items)
+            borefield.append(_borehole)
+
+        self.gfunc = gFunction(
+            borefield,
+            alpha,
+            time=time,
+            boundary_condition=boundary_condition,
+            options=options,
+            method=solver_method,
+        )


@Myoldmopar responding to this comment (https://github.com/mitchute/pygfunction/pull/4/files#r1834563947) the gFunction class needs a list of Borehole class instances. All this is doing is setting up the init arguments which that class needs. Those parameters are here: https://github.com/mitchute/pygfunction/pull/4/files#diff-3451cc474d5698222168f66a293339a6bd296ee2c56f56409866bbae99917a79R25

pygfunction does have some methods for building out these parameter lists for some GHE configurations, so maybe we can offload some of that there. For custom GHE configurations, I think we'll likely end up doing a lot of this ourselves.

Myoldmopar

@vtnate @mitchute

I made a few more comments on here. You are welcome to reject them if you feel they aren't valid. And also, I'm not intending on adding work to either of you, so if they are valid comments but you don't have time, I can jump in and get to work on them myself.

pygfunction/api.py

Myoldmopar · 2024-11-08T17:08:20Z

pygfunction/api.py

+from typing import Dict, Optional, Any, List
+
+
+class PYG(object):


I wonder about renaming the class. It kinda depends on what we expect this class to ultimately contain/do. Is it always going to be representative of a single borehole field? I'm open to suggestion, but this feels a bit vague.

Myoldmopar · 2024-11-08T17:10:01Z

pygfunction/api.py

+
+class PYG(object):
+
+    def __init__(self, borehole_config_list: List[List[float]],


I think it would be a bit nicer to pass in a list of structures instead of a list of list of floats. With the list of floats, our tools aren't able to help inform us of what each float should be. We could do some simple dataclasses that capture the different float members, and even add some helper functions to generate them for common cases. I just really want this API to be clean and self-documenting as much as possible.

Myoldmopar · 2024-11-08T17:10:53Z

pygfunction/api.py

+            options = {}
+
+        for borehole_config_items in borehole_config_list:
+            _borehole = Borehole(*borehole_config_items)


And in the little structures, we could have a to_list method that smashes the values down to be consumed by PyGFunction. That keeps that implementation detail away from API consumers.

tests/test_api.py

Myoldmopar · 2024-11-08T17:18:37Z

tests/test_api.py

+
+    # -- Assert
+    assert isinstance(pyg_1.to_list(), list)
+    assert isinstance(pyg_2.to_list(), list)


And before we get too much farther, we should probably grab a third-party source to generate some g-functions and do validation, or at least verification, on the g-function.

mitchute · 2024-11-11T23:15:43Z

@vtnate @Myoldmopar take a look. I still need to add some validation cases.

pygfunction/api.py

tests/test_api.py

Myoldmopar

This looks quite good. Very usable, in my opinion.

Myoldmopar · 2024-11-12T16:17:53Z

pygfunction/api.py

+        self.tilt_angle_list: List[float] = []  # TODO: Is this degrees? And I assume from vertical?
+        self.orientation_angle_list: List[float] = []  # TODO: Same question
+        self.x_coords: List[float] = []  # TODO: What are the units here, I'm assuming meters
+        self.y_coords: List[float] = []  # TODO: Same question


OK, modified this class just a bit. Now the constructor simply instantiates the lists. This will allow for us to create any variety of setup methods later on that can flexibly build out borehole fields, and the constructor won't need to change.

pygfunction/api.py

Myoldmopar · 2024-11-12T16:19:38Z

tests/test_api.py

+        def run_test(xy):
+            # compute g-functions
+            bh_field_params = BoreholeFieldParameters()
+            bh_field_params.initialize_borehole_field_generic(xy, self.height, self.depth, self.bh_radius)


This is the real change, just moving the arguments out of the constructor to free us up to make changes later without breaking constructor calls.

mitchute · 2024-11-12T16:41:47Z

tests/test_api.py

+        bh_field_params = BoreholeFieldParameters()
+        bh_field_params.initialize_borehole_field_generic(
+            xy_coords, self.height, self.depth, self.bh_radius, tilt_angle=20, orientation_angle=20
+        )
+        g = GFunctionGenerator(bh_field_params, self.alpha, self.time, solver_method="detailed")


@Myoldmopar should we just mash these into a single class? Surprisingly, I don't see a borehole field class in pygfunction. Maybe we just make this all a single basic class BoreholeField that does all this? It might be a bit more self-contained, and possibly a cleaner implementation.

bh_field = BoreholeField() bh_field.init_generic_field(args...) bh_field.generate_g_functions(args...)

mitchute · 2024-11-12T18:35:39Z

pygfunction/api.py

+        self.gfunc = gFunction(
+            self.as_config(),
+            alpha,
+            time=time,
+            boundary_condition=boundary_condition,
+            options=options,
+            method=solver_method,
+        )
+
+        return self.gfunc.gFunc
+
+    def to_list(self):
+        """returns a _list_ of g-function values"""
+        return self.gfunc.gFunc.tolist()


@Myoldmopar OK, I have combined the classes. Maybe one final (for now) question, is regarding the return types for each of these.

get_g_functions returns a numpy array; to_list converts that array to a list. Do these seem OK for now?

If it were my personal preference alone, I would say we just have get_g_functions return the plain list of floats and leave numpy out of it. It's easy enough for an API client to create a numpy array of it if they want it. It may be a slight inconvenience to a few people, but it's going to be completely insignificant in runtime and keeps up operating on plain python as much as possible. And then we don't need this to_list function anymore.

If we don't want to go that far, then I might still be inclined to remove this as_list function and just return a numpy array from get_g_functions. The client can still do whatever they like with it.

Make sense, hence my apprehension. I'll update that.

mitchute · 2024-11-30T04:34:05Z

doc/source/install.rst

@@ -4,11 +4,12 @@
 Setting up pygfunction
 **********************

-*pygfunction* uses Python 3.7, along with the following packages:
+*pygfunction* uses Python 3.8, along with the following packages:


@MassimoCimmino thanks, this looks great! One minor point - python 3.8 is past end of life, so perhaps in a separate PR this could be bumped up to a more current version.

Yes. I tend to update Python versions and dependencies on major and minor releases. This is one place where I had forgotten to update on the last version.

A little summary of my implementation if you want to discuss any of the changes:

The gFunction class now accepts Borefield objects. That did not require changes in the gFunction class (outside of isinstance checks) since the new class behaves like a list of boreholes for all operations done in pygfunction.

The Borefield.evaluate_g_function class method returns an array of g-function values. This was to align with the current gFunction.evaluate_g_function method.

I moved borefield creation functions (e.g. boreholes.rectangle_field) to the Borefield class. They needed to be re-implemented to generate arrays of geometrical parameters instead of lists of boreholes. The functions in the boreholes module now have a deprecation warning.

Examples are update to use the new interface for bore field creation.

Something to consider is if the boreholes.visualize_borefield should also be moved to the Borefield class. I remember we had a discussion about the dependency for matplotlib. An option is to include the method and import matplotlib at runtime within the method instead of the top of the module.

The PR is much larger than I expected. I will review it and make sure it is compatible with the planned changes for issue MassimoCimmino#210.

vtnate added 2 commits November 1, 2024 16:27

example api showing how to pass in config and return gfunction

5294981

test demonstrating use of api

21ccaa8

vtnate added the enhancement New feature or request label Nov 1, 2024

vtnate requested a review from mitchute November 1, 2024 22:32

vtnate self-assigned this Nov 1, 2024

vtnate commented Nov 4, 2024

View reviewed changes

pygfunction/api.py Outdated Show resolved Hide resolved

fix typing and allow options dict

f6241ab

vtnate marked this pull request as ready for review November 6, 2024 17:33

vtnate and others added 3 commits November 6, 2024 10:50

add todo in api test

97a7068

some cleanups

dce7030

fix type hinting that I broke

a4ec330

Myoldmopar reviewed Nov 8, 2024

View reviewed changes

pygfunction/api.py Outdated Show resolved Hide resolved

tests/test_api.py Outdated Show resolved Hide resolved

mitchute reviewed Nov 8, 2024

View reviewed changes

Myoldmopar reviewed Nov 8, 2024

View reviewed changes

mitchute added 3 commits November 11, 2024 15:56

incorporate suggestions

2c3cf40

fix typing

1c9d52a

more typing

a9a18df

vtnate commented Nov 11, 2024

View reviewed changes

pygfunction/api.py Outdated Show resolved Hide resolved

tests/test_api.py Outdated Show resolved Hide resolved

tests/test_api.py Outdated Show resolved Hide resolved

tests/test_api.py Outdated Show resolved Hide resolved

mitchute and others added 2 commits November 11, 2024 22:19

cleanup tests

3e0869a

Updated API file a bit

b2f5988

Myoldmopar reviewed Nov 12, 2024

View reviewed changes

mitchute reviewed Nov 12, 2024

View reviewed changes

mitchute added 2 commits November 12, 2024 10:05

add docstrings and other documenting comments

cbf728c

combine classes, update docs

ce9d520

mitchute reviewed Nov 12, 2024

View reviewed changes

mitchute and others added 3 commits November 12, 2024 11:59

return list

a67ad63

Initial implementation of Borefield class

a4fb763

Class method to evaluate g-function

f52c143

MassimoCimmino added 19 commits November 27, 2024 09:59

Change init to use arrays instead of boreholes

5f86520

Fix broadcasting of tilt and orientation

7c1bfc8

Fix borefield.from_file to call class init

8320711

Docstrings

5f31f6b

Remove api module

4707959

Test g-functions

fc78296

Test borefield.from_boreholes

f2d4341

Implement bore field creation methods

4806d16

Deprecation warnings for old bore field functions

d5374ce

Fix Borefield.from_file

1a5f0c1

Update examples

59e0d99

Boreholes can have negative tilt

6027e42

Save field to file

f999eac

Fix dense and box fields

2010311

Update tests

4364c06

Remove blank space

0645d57

Import Self from typing_extensions

8f5543c

Update CHANGELOG.md

c512fc7

Update requirements

5972850

mitchute reviewed Nov 30, 2024

View reviewed changes

MassimoCimmino added 2 commits November 30, 2024 15:09

Review changes

e3c92d7

Re-order methods

34d5543

MassimoCimmino mentioned this pull request Nov 30, 2024

Create API for computing g-function values from static inputs MassimoCimmino/pygfunction#308

Merged

MassimoCimmino added 3 commits December 1, 2024 05:26

Implement Borefield.visualize_field

033c9ac

Typing for Borefield.visualize_field

1084a09

Final review

a37a8ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential API for PyGFunction #4

Potential API for PyGFunction #4

vtnate commented Nov 1, 2024

mitchute commented Nov 8, 2024

mitchute Nov 8, 2024

Myoldmopar left a comment

Myoldmopar Nov 8, 2024

Myoldmopar Nov 8, 2024

Myoldmopar Nov 8, 2024

Myoldmopar Nov 8, 2024

mitchute commented Nov 11, 2024

Myoldmopar left a comment

Myoldmopar Nov 12, 2024

Myoldmopar Nov 12, 2024

mitchute Nov 12, 2024 •

edited

Loading

mitchute Nov 12, 2024

Myoldmopar Nov 12, 2024

mitchute Nov 12, 2024

mitchute Nov 30, 2024

MassimoCimmino Nov 30, 2024

		from typing import Dict, Optional, Any, List


		class PYG(object):


		class PYG(object):

		def __init__(self, borehole_config_list: List[List[float]],

Potential API for PyGFunction #4

Are you sure you want to change the base?

Potential API for PyGFunction #4

Conversation

vtnate commented Nov 1, 2024

mitchute commented Nov 8, 2024

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchute commented Nov 11, 2024

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchute Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchute Nov 12, 2024 •

edited

Loading