openterrace / openterrace-python Goto Github PK

On a repository's GitHub page, it's possible to add a URL in the top right - this makes it easy to navigate to the documentation page. I suggest adding this to this repo. See the screenshot from the pandas GH page as an example:

Right now the files for the JOSS submission is cluttering the repo. I suggest putting the files in a folder called "paper".

I'm not exactly sure how you need to specify this to joss, but you can probably find the info here: https://joss.readthedocs.io/en/latest/index.html

I installed via pip install openterrace into my existing conda environment in Windows 10.

In Tutorial 1, I get this error:

ot = openterrace.Setup(t_end=60*10, dt=0.01, sim_name='tutorial1')
AttributeError: module 'openterrace' has no attribute 'Setup'

In the other Tutorials, I get this error:

ot.fluid.select_bc(bc_type='fixedValue',
in select_bc
raise Exception("bc_type \'"+bc_type+"\' specified. Valid options for bc_type are:", valid_bc_types)
Exception: ("bc_type 'fixedValue' specified. Valid options for bc_type are:", ['neumann', 'dirichlet', 'dirichlet_timevarying'])

After changing the BC to neumann or dirichlet, I then get this error:

ot.bed.select_domain_shape(domain='sphere_1d', R=R)
in select_domain_shape
self.domain.shape = self.domain.shape(kwargs)
TypeError: 'numpy.ndarray' object is not callable

Any ideas how to fix this or what is going wrong? Is the installation incomplete?

In my folder \anaconda3\Lib\site-packages\openterrace I have a folder called boundary_conditions which does not exist in the repository here on github. In that, there are 3 empty(!) .py files: fixedValue.py, timeVaryingFixedValue.py and zeroGradient.py

I can run the \openterrace\tests\test_functions.py, added this code to its end:

def run_all_tests():
    for test_cls in [TestDiffusion, TestConvection]:
        test_instance = test_cls()
        for method in dir(test_instance):
            if method.startswith('test_'):
                getattr(test_instance, method)()

if __name__ == '__main__':
    run_all_tests()

And the 3 output .svg files seem fine:

It's fairly standard for python packages to support returning the pacake version by the command .__version__. This currently isn't supported by the package.

    import openterrace
    openterrace.__version__
    >>> AttributeError: module 'openterrace' has no attribute '__version__'

A minor comment that you can feel free to ignore, but there seems to be missing a space around the tutorials and number, e.g., "Tutorial1" should be "Tutorial 1".

Currently, the requirements for building the documentation is hardcoded in the GitHub action docs.yml.

This is not ideal for other users who want to build the documentation. Also, I highly suggest specifying a version for all the major documentation dependencies to avoid bugs and undesirable changes down the road as new versions of packages are released.

This is probably most easily achieved by having a requirements.txt file in the docs folder.

First, I think it would be very useful including this figure from the JOSS paper in the documentation:

Second, it seems that there is an issue with the numbering. The text states that "For each node, the bed material is discretised in M nodes", however if the nodes start at 0 and ends at M, there will be M+1 nodes. I suggest either starting from 1 or ending at M-1.

This issue includes comments that should be addressed to resolve the "Software Paper" portion of the review checklist:

• The summary describes the purpose of the software (studying thermal storage) but not the functionality provided by the software. A brief description of the uses of the software in studying thermal storage should be added.
• The statement of need describes the intended uses and intended users of the software, but not the relation to other work. Is there other software used by researchers for similar purposes? Are there relevant publications of related scientific/engineering work that should be referenced to provide context?
• State of the field: Not adequately addressed, can be resolved by answering the questions above in the paper.
• Quality of writing: There is no problem with the quality of writing. However, the second figure should be referenced/described in the text and/or a caption should be provided so that the reader understands the significance.

The current references are good, but more may need to be added in resolving these points.

Is there a reason to sill have the testPyPi.yml file? If there is, then it's perfectly ok to keep it.

The CITATION.cff file should be updated to point to the final JOSS paper once/if this is accepted.

This issue should not be addressed right now but is opened as a reminder once the JOSS review process is complete.

I noticed that the parameter arguments use a mix of camel case (e.g., fixedValue) and snake case (e.g., central_difference)

For example this example in the tutorials:

    fluid.select_schemes(diff='central_difference_1d', conv='upwind_1d')
    fluid.select_initial_conditions(T=273.15+20)
    fluid.select_massflow(mdot=0.1)
    fluid.select_bc(bc_type='fixedValue',

The tutorials generate some form of plots but these do not render in the documentation.

Statement of need:

• Adding a bit more detail on the uses of the code on the landing page of the documentation would be helpful

Installation instructions:

• See installation comments in #167

Example usage:

• Sample results are missing for Tutorial 7
• It would be helpful to have a bit more detail on what each tutorial is, as well as a sentence or two describing the results in addition to the figures.

Automated tests:

• There are automated tests that run through a github workflow when PRs are merged. However, this is largely undocumented. Adding a bit to the documentation about these tests and how users/potential developers can run them would be helpful.

It seems like the package does not have a version at the moment (I noticed 0.0.0 in the pyproject.toml file).

I recommend using Semantic versioning https://semver.org and making a tagging release. I suggest starting at version 0.1.0.

If you need help with this process, let me know.

Hi,

I'm impressed by the work done but I can't get OpenTerrace to run...

In the documentation the installation steps are missing but it would be a pleasure to work with you to make the installation available with Conda-forge

I'm unsure of what the files in the temp folder are used and the name of the folder does not give an indication. Consider renaming it scripts? Are the files used for documentation?

Installation

• The installation instructions using pip should specify a required version of Python (instructions fail for 3.10 or less). In general, it might be helpful to note major dependencies/versions in the documentation and/or point users to the pyproject.toml file for more information on dependencies
• editable installation instructions should tell users which steps can be skipped if they already have a conda installation

Functionality

• GPU functionality is claimed, but no instructions are provided. I'd like to be able to run on GPU and CPU and see the performance difference (once my GPU machine is back online from maintenance)
• The verification against an analytical solution included in the documentation is good. Can the ability to reproduce this be added to the github repo? Adding additional verification and validation showing the correct order of convergence and testing other functionality would strengthen the software, but is not blocking for acceptance. EDIT: actually now I see these are generated from the tests. Adding a bit more detail in the documentation to clarify this may be sufficient

Performance

• “approaches the speed of compiled C or FORTRAN code” in the README is a performance claim, but can't really be verified. I'd recommend rephrasing this statement so it is not making a specific performance claim.