Skip to content
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

Issue33 commong function interface #84

Merged
merged 28 commits into from
May 17, 2021

Conversation

MassimoCimmino
Copy link
Owner

@MassimoCimmino MassimoCimmino commented Apr 8, 2021

This closes #33, closes #54, closes #85.

I am opening this PR to get some feedback and review on the new interface (#33). It is mostly complete. The remaining tasks are to

Some highlights:

  • I removed functions from pygfunction.gfunction, namely load_history_reconstruction, _borehole_segments, _temporal_superposition as they are now part of the new classes.
  • I modified the existing examples to use the new interface.
  • There are new visualization features in the gFunction class. These are demonstrated in the modified examples, e.g. uniform_temperature.py.

@MassimoCimmino MassimoCimmino marked this pull request as draft April 8, 2021 21:40
@MassimoCimmino MassimoCimmino marked this pull request as ready for review April 8, 2021 21:41
@MassimoCimmino MassimoCimmino added this to the v2.0.0 milestone Apr 8, 2021
@MassimoCimmino MassimoCimmino marked this pull request as draft April 8, 2021 21:42
@MassimoCimmino MassimoCimmino force-pushed the issue33_CommongFunctionInterface branch from 845d7cf to 7894902 Compare April 17, 2021 01:23
@MassimoCimmino
Copy link
Owner Author

The PR is ready. I squashed commits and rebased the history (trying out for #83).

@MassimoCimmino MassimoCimmino marked this pull request as ready for review April 17, 2021 01:33
j_c_cook and others added 9 commits April 16, 2021 21:48
This introduces nominal conditions (m_flow ans c_p) to the Network
class. These are used at initialization to initialize the values of
the Network coefficients.

This is needed to simplify the interface to the new gFunction class, so
that m_flow and c_p are not needed to evaluate the g-function.
The g-function template now calls the existing functions.

Future changes will now be able to be tested.
The new Solver classes will be called by the gFunction class to
calculate the gFunction. This will facilitate the implementation of new
calculation methods as they are developed in the literature.

The _BaseSolver class implements the `solve` function, which is aimed
to be applicable independent of the solution method. Method-specific
tasks can be implemented in their own classes.
The gFunction class can now plot the borehole temperatures and heat
extraction rates (both as a function of time and the profiles over the
length). These are often shown in publications and are useful to
interpret the result of g-function calculations.

In the past, modified versions of pygfunction were used to output these
for publications. The gFunction class and the new visualization methods
add this functionality natively while limiting the memory use through
the `profiles` options.
Make sure the provided inputs are valid and automatically choose the
boundary condition depending on the provided `boreholes_or_network`,
if not specified by the user (this is simply for conveninence).

This also makes use of the `find_duplicates()` function introduced in
#81 to make sure the bore field is valid and the g-function can be
properly calculated.
@MassimoCimmino MassimoCimmino force-pushed the issue33_CommongFunctionInterface branch from 502f64a to aad279c Compare April 17, 2021 01:49
@MassimoCimmino
Copy link
Owner Author

@j-c-cook Could you take a look at this PR? It is pretty much complete from my side.

@j-c-cook
Copy link
Contributor

@MassimoCimmino I haven't forgot about this. We are finishing up the semester down here and its a wild time. I will get around to this as soon as practical.

If at some point you feel the need to merge it and I haven't gotten around to looking closely, please feel free to merge.

@j-c-cook
Copy link
Contributor

j-c-cook commented May 13, 2021

I was studying your equation 18 from Cimmino (2018).

image

In master you pass dh_ij to the _temporal_superposition. However, in this branch you are passing h_ij and computing a dQ to find the dimensionless borehole wall temperature.

Did you decide to do this because you thought it was less work on the CPU? Does this provide both a speed increase and a reduction in memory? I remember you saying somewhere that you're getting the right results with this new code, so that leads me to believe this formulation works. Why is it that equation (18) has Q_reconstructed(Tau_k^prime), but the code makes use of Q[:, nt-it-1]?

Edit: It’s been a while since I looked at this, and now I see equation 37, which explains what is in master.


Massimo Cimmino (2018) Fast calculation of the g-functions of geothermal borehole fields using similarities in the evaluation of the finite line source solution, Journal of Building Performance Simulation, 11:6, 655-668.

@MassimoCimmino
Copy link
Owner Author

MassimoCimmino commented May 13, 2021

The change is motivated by #85. In the PR, dh_ij[:nSources, :nSource, :nt] is no longer computed and instead we only have h_ij[:nSources, :nSources, :(nt+1)] and h_dt[:nSources, :nSources]. We previously had all 3 plus the stored array in the interp1d object.

The two formulations of the temporal superposition process are equivalent : dT = convolution(dh, Q) = convolution(h, dQ). I don't see it affecting the CPU time in any significant way but made no attempt to quantify this. The implementation of _temporal_superposition() is probably not optimal (it creates a new array of size nSources x nt every time step). This is something that could be revisited after #78. The two remaining computational bottlenecks in that branch (for large fields) are the construction of h_ij from the already computed FLS and the solution of the system of equations (which the optimization of _temporal_superposition() may help).


On another note, I noticed there are now unused functions in the heat_transfer module : thermal_response_factors, similarities, _similarities_group_by_distance, and _similarities_one_distance. These were all transferred to the Solver classes. Unless you have any use for them, I propose they be removed.

Existing functions now call the gFunc class instead of having their
separate implementation.

Existing examples are changed to use the new gFunction class. Examples
are also extended to demonstrate the new visualization features.
Solver classes are not meant to be directly accessible to the user. They
also made the documentation very hard to read.

Moved to end of file.
The heat extraction rates and borehole wall tempratures only need to be
stored after the g-function is evaluated.
@MassimoCimmino MassimoCimmino force-pushed the issue33_CommongFunctionInterface branch from 08975c0 to 916c658 Compare May 15, 2021 21:02
@MassimoCimmino MassimoCimmino merged commit 558e367 into master May 17, 2021
@MassimoCimmino MassimoCimmino deleted the issue33_CommongFunctionInterface branch November 12, 2021 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants