Skip to content

Conversation

Czarified
Copy link
Contributor

This PR adds some basic tests with the hypothesis library, as well as a pytest suite for accuracy against an unsymmetric beam in biaxial bending from Peery. Results pass within 0.5% of theoretical (possibly less... that was good enough for me). I also added a new docs page explaining the tests, showing some code examples, and showing the latest results from the pytest log file.

Added hypothesis testing functions.
Still WIP, but have to stop right now. Got basic geometry checks with the Peery example. Need to check stresses at the specified recovery points.
Still don't know exactly how to pull stress results at a recovery point... Until then, the test will make sure the text_result formula gives correct results according to the stress recovery points from the z_section class.
added additional text_result assertion, and fixed the perfect_result for Point C.
Computes the section, and all streses. Computed streses from FEA within .5% of Peery results.
Added docs pages. Still WIP.
Created and built new docs with the pytest example and results.
Added the tolerance for accuracy to the stress function.
@Czarified
Copy link
Contributor Author

Czarified commented Feb 2, 2021

I'm fine with merging this as-is, but I am also willing to keep expanding it, if anyone has other suggestions. I mentioned the other (hypothesis) tests on the docs page, but didn't show anything about them or save a report from running them.

@Spectre5
Copy link
Contributor

Spectre5 commented Feb 2, 2021

I have not had a chance to look at this yet, so my comments below are not directly related to your PR. They are just some observations of mine.

It is best practice to not store the documentation build in the repository, only the docs source. This is especially true of binary data (e.g. images) which cannot be diff'd with git. This is even more true if you re-create the images/build regularly, like with every push to master or every library release. Other projects have various ways of dealing with this, usually pushing the documentation build artifacts directly to some 3rd party service.

I took a closer look around at the pyvista repository last night. What they do in their ci file is automatically delete their dummy examples and docs repositories and then push the latest contents to them, with all examples re-run and images updated for the doc. They use notebooks to accomplish this, but you could probably do it without too. I like this idea as it keeps only source in your main repository. Further, their documentation website is actually hosted by github pages and is directly served from that pyvista-docs repository. For anyone unaware, you can use custom domain names to server github pages, see here for some details. I know that section-properties currently uses readthedocs, but I bet we could do something similar where we push the documentation to a second "docs" repository and then feed that to readthedocs.

@Czarified
Copy link
Contributor Author

@Spectre5 , I think the docs change you're talking about should probably be its own PR. I don't really understand the downsides to storing documentation the way Robbie originally set it up. Feel free to start a discussion on docs for this, in our fancy new discussions area.

@Spectre5
Copy link
Contributor

Definitely fair point, the discussion would be a better place to make those comments. I'll put it there

Copy link
Contributor

@Spectre5 Spectre5 left a comment

Choose a reason for hiding this comment

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

So in this PR, you remove the entire docs/build directory directory that @robbievanleeuwen currently has in the project. Was that intentional?

I think there is no need to include the peery.log file either. That will be shown to a user that runs the tests with pytest anyway.

@Spectre5
Copy link
Contributor

Spectre5 commented Feb 21, 2021

As for the tests themselves, test_Perry.py runs fine and passes for me, but I did get an error on test_hyp.py:

sectionproperties/tests/test_hyp.py:45: 
----------------------------------------
d = 0.01, b = 0.01

    @given(
        d=st.floats(min_value=.01, max_value=10),
        b=st.floats(min_value=.01, max_value=10)
    )
    def test_rectangle_mesh_size(d, b):
        '''
        This function verifies that the ValueError is raised when mesh size is
        set too small.
        '''
        with pytest.raises(ValueError):
            xsect = sections.RectangularSection(d, b)
>           mesh = xsect.create_mesh([0.1/251])
E           Failed: DID NOT RAISE <class 'ValueError'>

sectionproperties/tests/test_hyp.py:55: Failed
----------------------------------------
Falsifying example: test_rectangle_mesh_size(
    d=0.01, b=0.01,
)

@Czarified
Copy link
Contributor Author

Czarified commented Feb 22, 2021

So in this PR, you remove the entire docs/build directory directory that robbievanleeuwen currently has in the project. Was that intentional?

That was not intentional... I didn't notice that. I don't know why it didn't sync up. I'll try again soon.

I did get an error on test_hyp.py

I need to re-run this on my local branch and check. I may have included this with an error check in the geometry.create_mesh() method. If we don't want to include that error handling, then I need to remove this test. If we do, I need to make sure I included the right file in this PR.

Some stuff for me to check later today after work! Thanks for taking a look!

I think there is no need to include the peery.log file either. That will be shown to a user that runs the tests with pytest anyway.

I included this, because the docs page actually reads the file directly. So if we have CI run pytest on every release, the docs page should automatically update to show the "latest" state of passing all tests.

@Spectre5
Copy link
Contributor

Spectre5 commented Feb 23, 2021

Although I pointed out that you've removed the entire build directory with the documentation, I actually support doing that (see my first comment on this PR)! But I figured that you didn't do it intentionally. If someone wants to get the docs, they can just clone the repo and build it themselves or (more likely) go to readthedocs to see it. So there is no reason to store the docs build in the repository and it is considered poor practice. Note that the way readthedocs works, is that they essentially clone your repository and then build the documentation, so it doesn't use the "build" directory that is committed here.

This brings up another point --- @robbievanleeuwen, can you enable the docs to be built on pull requests too, please. It should be an option in the readthedocs settings. I've seen other projects do it and it makes a lot of sense so that we can see what the documentation would look like on new pull requests before actually merging it!

@Czarified
Copy link
Contributor Author

I will revisit this, and add some more unit tests as available. Need to upgrade everything to v2, and get acquainted with the changes to the codebase first. Closing this PR. Will make a new one when ready.

@Czarified Czarified closed this Jan 17, 2022
@robbievanleeuwen
Copy link
Owner

@Czarified much appreciated! Sorry there was a bit of a lull at the start of last year, I think this will be a really great addition 🚀

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.

3 participants