Skip to content

Conversation

@MassimoCimmino
Copy link
Owner

@MassimoCimmino MassimoCimmino commented Jan 24, 2025

This follows the work in #308 and works towards fully integrating the Borefield class into the g-function module and solvers to close #210 and #211.

This requires a major refactoring of the heat_transfer module and the different g-function solvers. Here is an overview of the ongoing work (and the reasoning) on this issue:

  • Move solvers into their own solvers module. The gfunction module was too large and this allows to isolate each class into their own script to facilitate the work on different parts of pygfunction. This will facilitate the addition of new solvers in the future.
  • Split the heat_transfer module into multiple scripts. This script was also too large. Different functions can naturally be classified into files for the vertical FLS and the inclined FLS. This change will also facilitate the addition of other heat transfer solutions in the future (e.g. for Short-term correction using the cylindrical heat source #44 and Groundwater advection #219).
  • Refactor heat_transfer functions to use Borefield objects as inputs instead of long lists of geometrical parameters. This will simplify unnecessary code in the solvers that formats geometrical parameters before calls to heat_transfer functions.
  • Refactor the Detailed, Similarities and Equivalent solvers to make use of the Borefield class. This should greatly improve the readability of the code.
  • Refactor Network to use the Borefield class
  • Use type annotations in refactored modules. We already used them in the borefield module for Create API for computing g-function values from static inputs #308. Since the solvers will be almost entirely re-implemented, might as well make progress on Type annotations and mypy #178.
  • Update unit tests.
  • Update documentation.

This addresses some of the issues pointed out by @ikijano in #257. It has become increasingly difficult to implement new features as they multiply (it was not possible to predict the vast array of methods we have today when we developed v2.0) and it is time to re-visit the implementation to make future development easier.

The work on issue #210 had been delayed since it first looked like it would negatively affect the computational time. Thankfully, it now looks like there might even be some (not that significant but still not always negligible) improvements.

Comment on lines +50 to +52
self.nBoreholes = np.max(
[len(np.atleast_1d(var))
for var in (H, D, r_b, x, y, tilt, orientation)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider an assertion for when len(x) != len(y)?

What about enforcing H, D, r_b, tilt and orientation to match len(<var>) == len(y) or len(<var>) == 1?

Copy link
Owner Author

Choose a reason for hiding this comment

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

An assertion would certainly be important here. The goal was to mimic numpy array broadcasting rules and allow for example x = [0., 5., 10.], y = 0. if 3 boreholes are on the same row.

This comment was marked as outdated.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The Borehole class can represent both a segment of a borehole or the borehole itself. Likewise, the collection of parameters in Borefield can represent segments or boreholes. With that in mind, segments along a vertical borehole share the same coordinates (x, y) and only require H and D arrays at instantiation.

One way to approach this would be to use b = np.broadcast(H, D, r_b, x, y, tilt, orientation) to make sure that the inputs are broadcastable to a common shape with b.nd <= 1. The number of boreholes is then nBoreholes = b.size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to make use of any ideas contained in commit j-c-cook@bd6e467. I've enjoyed the back and forth and little bit of collaboration here. I'll likely drop out of discussion now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I adapted your changes in #322. I removed the try / except clauses since numpy.broadcast raises an error if the arguments cannot be broadcast.

@j-c-cook
Copy link
Contributor

The following example shows that a borehole field of size 1 can be both a Borefield and a Borehole. Have you thought about deprecating the Borehole object?

    x = np.array([0.])
    y = np.array([0.])
    borefield = gt.borefield.Borefield(H, D, r_b, x, y)
    print(type(borefield))
    print(type(borefield[0]))

Outputs:

<class 'pygfunction.borefield.Borefield'>
<class 'pygfunction.boreholes.Borehole'>

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.

New Borefield class

3 participants