[JOSS Review] Functionality + documentation

[wouterpeere]

[JOSS Review] Functionality + documentation

[nmstreethran]

Thanks @wouterpeere! It's nice to have a separate table for the comparison documents.

That's it from me. I'll close this issue now and update the editor.

[wouterpeere]

Hi @nmstreethran, thank you for your comments.

W.r.t. the installation: I wrote a small paragraph under the installation section with a link to the official python documentation and an article on venv that I found very clear. I also included a PyPi-badge, thank you for this tip!

W.r.t. the functionality: I wrote most of the API documentation in the example documents, but perhaps that is not the best location. Would it be better if I took some code snippets and placed them directly on the README? I can then also directly refer to the different examples.

W.r.t. the automated test. The scripts in the validation folder are more about the 'correctness' of the results from the package than about whether or not the package is installed correctly. I will make a test for that and explain (perhaps also under the installation section) how one can run these tests.

W.r.t. the examples. Yes, I will refer to them in the README (like mentioned hereabove).

[nmstreethran]

Hi @wouterpeere.

W.r.t. the functionality: I wrote most of the API documentation in the example documents, but perhaps that is not the best location. Would it be better if I took some code snippets and placed them directly on the README? I can then also directly refer to the different examples.

Yes, I think that would be sufficient for the functionality and example documentation. You could create a table of the functions and their respective examples, if appropriate.

Your other suggestions re: installation and tests sound good to me.

Many thanks,
Nithiya

[wouterpeere]

Hi @nmstreethran

I implemented the suggested changes!

[nmstreethran]

Hi @wouterpeere, thank you for implementing the changes.

I don't see the requirements defined anywhere in the package (other than the list in the README). When I follow the installation instructions and install GHEtool with pip or build from source, the requirements are not installed automatically. Could you define them under install_requires as shown here and here? Pytest should also be included in the list of development requirements (i.e. under extras_require).

I also noticed that the PyPI version of GHEtool is 2.0.2, but setup.cfg says 2.0.1.

Could you clarify how the requirement for Tkinter is satisfied?

[nmstreethran]

@wouterpeere Can I also suggest you to use relative links to repository files in the README? Currently, you're using the full path, e.g. https://github.com/wouterpeere/GHEtool/blob/main/GHEtool/Examples/Custom_Borefield_Configuration.py; this always points to the main branch. Use relative links, e.g. GHEtool/Examples/Custom_Borefield_Configuration.py.

[nmstreethran]

@wouterpeere Additionally, I had an issue with installing pygfunction as one of its dependencies, CoolProp, fails to install. Looks like this is a problem when installing the current version of CoolProp (v6.4.1) with Python >= 3.9. (I'm on Arch Linux.) Installing from source/testpypi resolved this issue:

pip install -i https://test.pypi.org/simple/ CoolProp==6.4.2.dev0
pip install GHEtool numpy scipy matplotlib pygfunction openpyxl pandas

It may be worth pointing this out in your README (as you say the code is tested with Python 3.9; the other option is to downgrade to 3.8).

See:

• CoolProp/CoolProp#1992
• CoolProp/CoolProp#2119

[nmstreethran]

Update re: Tkinter @wouterpeere

I managed to get it to work by first installing Tk on my system. The instructions are platform-specific so I think you can add this to the README with a link to the TkDocs tutorial for installing Tk: https://tkdocs.com/tutorial/install.html

[wouterpeere]

Hi @nmstreethran, thanks for all the comments!

@wouterpeere Can I also suggest you to use relative links to repository files in the README? Currently, you're using the full path, e.g. https://github.com/wouterpeere/GHEtool/blob/main/GHEtool/Examples/Custom_Borefield_Configuration.py; this always points to the main branch. Use relative links, e.g. GHEtool/Examples/Custom_Borefield_Configuration.py.

This has been changed!

I don't see the requirements defined anywhere in the package (other than the list in the README). When I follow the installation instructions and install GHEtool with pip or build from source, the requirements are not installed automatically. Could you define them under install_requires as shown here and here? Pytest should also be included in the list of development requirements (i.e. under extras_require).

This has been implemented and uploaded to test PyPi. You can install/test it using the command:

pip install --extra-index-url https://test.pypi.org/simple/ GHEtool==2.0.3rc1

I also noticed that the PyPI version of GHEtool is 2.0.2, but setup.cfg says 2.0.1.

This has been changed to 2.0.3rc1. After the whole JOSS process is finished, I will set it to 2.0.3, since I realised that otherwise, there will still be a lot of versions to go.

Could you clarify how the requirement for Tkinter is satisfied?

This was necessary for an old functionality before the GUI existed. I now deleted it, so it is no longer needed.

Update re: Tkinter @wouterpeere

I managed to get it to work by first installing Tk on my system. The instructions are platform-specific so I think you can add this to the README with a link to the TkDocs tutorial for installing Tk: https://tkdocs.com/tutorial/install.html

This is hence also solved. Sorry for the inconvenience.

[wouterpeere]

@wouterpeere Additionally, I had an issue with installing pygfunction as one of its dependencies, CoolProp, fails to install. Looks like this is a problem when installing the current version of CoolProp (v6.4.1) with Python >= 3.9. (I'm on Arch Linux.) Installing from source/testpypi resolved this issue:

pip install -i https://test.pypi.org/simple/ CoolProp==6.4.2.dev0
pip install GHEtool numpy scipy matplotlib pygfunction openpyxl pandas

It may be worth pointing this out in your README (as you say the code is tested with Python 3.9; the other option is to downgrade to 3.8).

See:

• Installation errors with Python 3.9 CoolProp/CoolProp#1992
• Python bindings: Call for help from the community CoolProp/CoolProp#2119

I am aware of this problem. If you work with pygfunction==1.1.2, it works in python>=3.9. In the setup.cfg, I set the version of pygfunction to 1.1.2 in order to avoid any problems. I will add a small paragraph related to the problem you mentioned. Thanks for pointing it out!

[nmstreethran]

Hi @wouterpeere, thanks a lot for all the changes!

This has been changed to 2.0.3rc1. After the whole JOSS process is finished, I will set it to 2.0.3, since I realised that otherwise, there will still be a lot of versions to go.

Good call!

I have installed the package from testPyPI and ran the tests. Everything looks OK, all 9 tests pass but had 22 warnings. This seems to be from Pygfunction v1.1.2 using the deprecated np.asscalar(a) and has been fixed in v2.1.0 according to their changelog. I think this is fine though as it's not from your package. Just wanted to point this out.

I haven't finished going through the examples, but I'll get back to you on this, as well as the paper, tomorrow.

[wouterpeere]

@nmstreethran do you have any updates on the paper or the examples?

[nmstreethran]

@wouterpeere My apologies for the delay in getting back to you. I'll get the review completed ASAP.

I have some additional comments on the tests:

1. Can you clarify the need to use --pyargs GHEtool for testing? When I installed the development version from TestPyPI and ran the tests with the pyargs flag, no tests run. All tests run when it is omitted.

2. I installed the development version just now and ran the tests again. I found a new issue: the test_dynamicRb test fails (I've included the test output below). Numpy was updated to v1.23.0 six days ago and they have expired the asscalar function's deprecation (see https://github.com/numpy/numpy/releases/tag/v1.23.0). You'll need to restrict the Numpy version to <= v1.22.4 in setup.cfg. (It may be easier to just use a newer Python version with the latest version of pygfunction and the development version of CoolProp though, but I'll leave this to you.)

Pytest output

=============================================== test session starts ================================================
platform linux -- Python 3.8.13, pytest-7.1.2, pluggy-1.0.0
rootdir: /home/nms/Downloads/ghetool
collected 9 items                                                                                                  

GHEtool/GHEtool/test/test_GHEtool.py ........F                                                               [100%]

===================================================== FAILURES =====================================================
__________________________________________________ test_dynamicRb __________________________________________________

borefield = <GHEtool.GHEtool.Borefield object at 0x557355916f80>

    def test_dynamicRb(borefield):
        fluidData = FluidData(0.2, 0.568, 998, 4180, 1e-3)
        pipeData = PipeData(1, 0.015, 0.02, 0.4, 0.05, 0.075, 2)
        borefield.setFluidParameters(fluidData)
        borefield.setPipeParameters(pipeData)
>       assert round(borefield.size(100, useConstantRb=False), 2) == 52.57

GHEtool/GHEtool/test/test_GHEtool.py:103: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:528: in size
    result = self.sizeL2(H_init, quadrantSizing) if L2Sizing else self.sizeL3(H_init, quadrantSizing)
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:583: in sizeL2
    quadrant1 = sizeQuadrant1()
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:552: in sizeQuadrant1
    return self._Carcel  # size
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:501: in _Carcel
    L = (self.qh * self._Rb + self.qh * Rh + self.qm * Rcm + self.qpm * Rpm) / abs(self.Tf - self.Tg)
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:414: in _Rb
    return self.calculateRb()
env/lib/python3.8/site-packages/GHEtool/GHEtool.py:432: in calculateRb
    return gt.pipes.borehole_thermal_resistance(pipe, self.mfr, self.Cp)
env/lib/python3.8/site-packages/pygfunction/pipes.py:1870: in borehole_thermal_resistance
    a_out = np.asscalar(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

attr = 'asscalar'

    def __getattr__(attr):
        # Warn for expired attributes, and return a dummy function
        # that always raises an exception.
        try:
            msg = __expired_functions__[attr]
        except KeyError:
            pass
        else:
            warnings.warn(msg, DeprecationWarning, stacklevel=2)
    
            def _expired(*args, **kwds):
                raise RuntimeError(msg)
    
            return _expired
    
        # Emit warnings for deprecated attributes
        try:
            val, msg = __deprecated_attrs__[attr]
        except KeyError:
            pass
        else:
            warnings.warn(msg, DeprecationWarning, stacklevel=2)
            return val
    
        # Importing Tester requires importing all of UnitTest which is not a
        # cheap import Since it is mainly used in test suits, we lazy import it
        # here to save on the order of 10 ms of import time for most users
        #
        # The previous way Tester was imported also had a side effect of adding
        # the full `numpy.testing` namespace
        if attr == 'testing':
            import numpy.testing as testing
            return testing
        elif attr == 'Tester':
            from .testing import Tester
            return Tester
    
>       raise AttributeError("module {!r} has no attribute "
                             "{!r}".format(__name__, attr))
E       AttributeError: module 'numpy' has no attribute 'asscalar'

env/lib/python3.8/site-packages/numpy/__init__.py:311: AttributeError
============================================= short test summary info ==============================================
FAILED GHEtool/GHEtool/test/test_GHEtool.py::test_dynamicRb - AttributeError: module 'numpy' has no attribute 'as...
=========================================== 1 failed, 8 passed in 1.71s ============================================

[wouterpeere]

@nmstreethran Thank you for the comments

1. Can you clarify the need to use --pyargs GHEtool for testing? When I installed the development version from TestPyPI and ran the tests with the pyargs flag, no tests run. All tests run when it is omitted.

Personally, for me this flag is needed, otherwise no tests would run ... I found some explanation on it here.
However, I noticed that in v2.0.3rc1 (from TestPyPi) the test weren't actually installed (they are not embedded in the package). I now uploaded v2.0.3rc2 to testPyPi, which has the tests embedded. Can you please delete everything from v2.0.3rc1 (including the test folder) and install the newer version. This solved the problem for me ...

2. I installed the development version just now and ran the tests again. I found a new issue: the test_dynamicRb test fails (I've included the test output below). Numpy was updated to v1.23.0 six days ago and they have expired the asscalar function's deprecation (see https://github.com/numpy/numpy/releases/tag/v1.23.0). You'll need to restrict the Numpy version to <= v1.22.4 in setup.cfg. (It may be easier to just use a newer Python version with the latest version of pygfunction and the development version of CoolProp though, but I'll leave this to you.)

I noticed it myself. I think that, since many people will be willing to upgrade to a higher version of Numpy, your latest suggestion is better. I will change the installation instructions in the README with the next push.

[nmstreethran]

Thanks @wouterpeere. Both pytest and pytest --pyargs GHEtool work for me now. Here's what I did with the present state of the package to ensure all tests pass using both these commands:

• installed GHEtool v2.0.3rc2 from Test PyPI
• installed CoolProp v6.4.2.dev0 from Test PyPI
• installed pygfunction v2.1.0

Regarding the GUI, there is a precompiled EXE available. Is there a way users can compile it themselves? Is it possible to run it on platforms other than Windows?

[nmstreethran]

Regarding the paper, I found some issues with the bibliography file. I can open a PR to fix this if you prefer. I also noticed that there are two copies of the paper and bib file, in the root and the JOSS folder. Which ones are the main files? (are they symlinked?)

[nmstreethran]

Some minor general comments: some words should be capitalised - Python (in paper and README), Boydens Engineering (in paper)

[wouterpeere]

• installed GHEtool v2.0.3rc2 from Test PyPI
• installed CoolProp v6.4.2.dev0 from Test PyPI
• installed pygfunction v2.1.0

Perfect, I will use this in the README.

Regarding the GUI, there is a precompiled EXE available. Is there a way users can compile it themselves? Is it possible to run it on platforms other than Windows?

For now, but that is the expertise of @tblanke, only Windows is possible, but he was working on making it work on other platforms. Now, users are not able to compile it themselves, but I guess, @tblanke, if you include a script for it (and add some documentation to the README) they will be able to? I will make a separate issue for this!

[wouterpeere]

Regarding the paper, I found some issues with the bibliography file. I can open a PR to fix this if you prefer. I also noticed that there are two copies of the paper and bib file, in the root and the JOSS folder. Which ones are the main files? (are they symlinked?)

I duplicated them, since I was struggling to get it working. The one in the root is the correct one (I suppose).
What are the problems with the bibliography? If you want, you can use a PR to fix these issues, if that is not too much trouble for you.

[wouterpeere]

Some minor general comments: some words should be capitalised - Python (in paper and README), Boydens Engineering (in paper)

I will change it! The thing with 'boydens engineering' is that the legal firm name is without the capital letters, and they explicitly mentioned that it should be written that way...

[nmstreethran]

I think the instructions for compiling the GUI would be useful. But if it's too much work, then you can just make it clear in the README that the compiled GUI is Windows-only.

In the bibliography, the IEA report's link is broken, and it uses the note field for the conference location instead of the location field, among others. I also found the conference proceedings online and I think it would be useful to add those links in the bibliography. I'll create a PR and you can review the changes.

Regarding 'boydens engineering', if that's the legal name then you can just keep it that way. It's capitalised in the README though, so maybe you can change that?

[wouterpeere]

I think the instructions for compiling the GUI would be useful. But if it's too much work, then you can just make it clear in the README that the compiled GUI is Windows-only.

I will leave this up to @tblanke.

Regarding 'boydens engineering', if that's the legal name then you can just keep it that way. It's capitalised in the README though, so maybe you can change that?

I will change it!

In the bibliography, the IEA report's link is broken, and it uses the note field for the conference location instead of the location field, among others. I also found the conference proceedings online and I think it would be useful to add those links in the bibliography. I'll create a PR and you can review the changes.

Thanks!

[wouterpeere]

@nmstreethran thanks for the changes to the bibliography file. I've merged your PR!

[nmstreethran]

Hi @wouterpeere, sorry for the delay again.

I'm almost done with my review. Here are my remaining comments on the paper:

• You state that the sizing of borefields is typically slow (in the order of minutes) and that GHEtool can size a borefield in the order of tenths of milliseconds. Have you done any benchmarking for this? Any references regarding the time consuming process of borefield sizing would be useful as well.
• Statement of need
  • In the first sentence, the 2 in CO₂ should be a subscript
  • In the second paragraph, references would be useful, particularly for these: "borefields have a very high investment cost"; "In all research related to optimising borefield loads"
  • In the second last paragraph, "graphical user interface" can be shortened to GUI as you have defined this earlier
  • In the last paragraph, state the full form of HVAC
• In the second last point under Features, remove the asterisks in *.csv and *.xlsx or escape them using \, as they are causing the text in between to be formatted in italics

[wouterpeere]

Hi @nmstreethran thank you for your comments! No problem for the delay.

• You state that the sizing of borefields is typically slow (in the order of minutes) and that GHEtool can size a borefield in the order of tenths of milliseconds. Have you done any benchmarking for this? Any references regarding the time consuming process of borefield sizing would be useful as well.

I included a benchmark of some sort in the Speed_Comparison.py (under the Validation folder) for a field of 10 boreholes and one of 64. On my computer the, the one with 10 boreholes sizes 45.000 times faster with the precalculated data (1ms compared to 45sec) and with 64 boreholes it is 6.205 times faster (18ms, compared to 111s).

In literature, most discussions are about the time needed for the gfunction calculation, not so much the sizing. I found this paper (https://doi.org/10.1016/j.renene.2020.10.074) that states: "The construction of a g-function is case specific as it depends on the thermal response of each borehole, the field configuration and the ground thermal properties. This can become a computationally demanding process in problems where its construction is required thousands of times, such as in financial optimization-based design methods". I'm doubting whether referencing this article is an added value for this paper ...

[wouterpeere]

I implemented the other changes and added some more references to the paper.

[nmstreethran]

@wouterpeere Thanks! Can you add a link to the speed comparison script in the README?

I'm doubting whether referencing this article is an added value for this paper ...

I think the script you added is enough to verify the speed claim, so you don't need to reference that article.

In the last sentence of the Statement of need:

... to play an essential role in the energy transition and of the built environment.

I'm not sure if the "and" should be there...

The last sentence of the Comparison with existing tools should be changed to the following so that you don't repeat the author's name:

@Bernier has reviewed exhaustive literature of all existing borefield sizing methods and tools.

The CO2 subscript didn't seem to work. Can you try CO~2~ instead (as stated in the Pandoc manual)?

[wouterpeere]

@nmstreethran the changes have been implemented!
I added another table under the functionalities subsection for the comparison documents (both L2/L3 sizing and the speed comparison).