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

Enhance Visualize Pipe (draw borehole) #47

Closed
j-c-cook opened this issue Jul 2, 2020 · 8 comments · Fixed by #60
Closed

Enhance Visualize Pipe (draw borehole) #47

j-c-cook opened this issue Jul 2, 2020 · 8 comments · Fixed by #60
Milestone

Comments

@j-c-cook
Copy link
Contributor

j-c-cook commented Jul 2, 2020

The borehole object does not have an instance for drawing a borehole. Visually representing a borehole upon creation will provide a quick and easy check to make sure the borehole physically possible.

@MassimoCimmino
Copy link
Owner

Hello @j-c-cook.

Did you check the visualize_pipes() method of the _BasePipe class, here ?

I would agree that the visualize_pipes() method is not always practical, since the pipe objects require thermal properties for the borehole and soil.

Any reason why you considered a method within the Borehole class rather than a function in the boreholes module?

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Jul 7, 2020

I did not notice visualize_pipes before writing this code. Is this in an example somewhere?

In borehole because its one borehole top view, however the placement inside of _BasePipe is nice.

My one complaint is that your line thickness on the u-tubes is too thick. To me the purpose of visualizing this top view is a double check that your borehole design makes sense. If the lines detract from that task, then they are too thick.

I set the other variables to 1 (0 breaks the code) in the following minimal example:

# ---- Minimal input for designing a borehole from top view
    # Borehole dimensions
    H = 400  # Borehole length (m)
    D = 5  # burial depth (m)
    r_b = 0.0875  # Borehole radius (m)
    # Pipe dimensions
    rp_out = 0.0133  # Pipe outer radius (m)
    rp_in = 0.0108  # Pipe inner radius (m)
    D_s = 0.029445  # Shank spacing (m)

    # Single U-tube [(x_in, y_in), (x_out, y_out)]
    pos_single = [(-D_s, 0.), (D_s, 0.)]
    # define a borehole
    borehole = gt.boreholes.Borehole(H, D, r_b, x=0., y=0.)
    # variables not needed to visualize borehole
    k_s = 1
    k_g = 1
    R_f_ser = 1
    R_p = 1

    SingleUTube = gt.pipes.SingleUTube(pos_single, rp_in, rp_out,
                                       borehole, k_s, k_g, R_f_ser + R_p)
    fig = SingleUTube.visualize_pipes()

    fig.savefig('mc.pdf')

Cimmino top view:
mc.pdf
Cook top view:
400_m.pdf

@MassimoCimmino
Copy link
Owner

I cannot find an example for the visualization. It seems there is none.

Should we work on _BasePipe, or do you think there is still a need for a separate function/method in Borehole?

I agree, the line thickness is too large for the purpose of validating the geometry. Would 0.75 pt (down from 1.5 pt) be better (mc-075.pdf) ?

I see a few features in #48 that is probably useful in your activities, mainly the show_plot (for cluster computations) and save_plot options. These could also be integrated in boreholes.visualize_field().

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Jul 7, 2020

Another point:
Your visualize_field() method plots everything in black ink. My draw_borehole method plots the outer borehole, the pipe outer radius and the pipe inner radius all a different color. Given that visualizing the top view is a double check, more information can be gathered using different colors. Also, my plot should have a legend indicating rpo, rpi, and rb. It could happen that a user has defined r_po as r_pi unintentionally, in which case the effort of this double check is wasted.

Looks to me like there are good things about both methods. The placement makes more sense inside of _BasePipe. An example could show that the additional variables can be dummy variables set to 1; not a deal breaker.

A line width of 0.75 point looks to be about default, maybe it should be even less.

Yeah #48 has some useful ideas, but your approach of returning fig could be potentially be useful for unforeseen reasons. Although the downsides being:

  • a user might not close() the figure, and while there should be warnings if there are 1000 figures open, maybe the error handling misses and there's unused data suspended
  • if a user is using a non-GUI backend, they will have to decide themselves on how to eradicate the errors on a non-GUI machine

Here's a list of improvements neither of our functions include:

  • low point line width (maybe less than default)
  • a legend detailing which color corresponds to the borehole, pipe and outer diameters
  • boolean operator for returning fig (default: False implies closing fig out)

Edit: I should have time to get around to combining these ideas into _BasePipe.visualize_pipe() sometime this week or this weekend, if you don't knock it out first.

@j-c-cook j-c-cook changed the title Draw Borehole Enhance Visualize Pipe (draw borehole) Jul 7, 2020
@MassimoCimmino
Copy link
Owner

In parallel, we could run a check at the initialization of the pipe object to validate the geometry, which is not done at the moment.

I can work on a class method _check_geometry for the _BasePipe to verify the geometry and return an error if any of the following are False:

  • r_po > r_pi
  • r_pi > 0.
  • k_s > 0. and k_g > 0. and R_fp > 0.
  • J >= 0
  • distance(pipe_i, pipe_j) >= 2*r_po
  • distance(pipe_i, origin) <= borehole.r_b - r_po

I should be able to quickly implement this in the next few days while you work on the visualization.

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Jul 7, 2020

Yes! Great, thank you.

@MassimoCimmino
Copy link
Owner

The _check_geometry method is implemented in 05bfd6.

@j-c-cook
Copy link
Contributor Author

I think it might be nice to have the borehole thermal resistance calculated and printed out in that custom_borehole.py example file I added.

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 a pull request may close this issue.

2 participants