NZS3101 design code

[Agent6-6-6]

Hi @robbievanleeuwen

Still have some minor updates to perform, some more formal verification of results, updating/correcting of docstrings, review of built docs, create some examples, etc..... but the bones are present to perform 3 types of analysis:-

• Normal design analysis: Normal material properties, normal strength reduction factors

• Capacity Protected Element analysis: Normal material properties, strength reduction factor of 1.0

• Overstrength capacity analysis: Likely upper limit material properties (steel strength multiplied by overstrength factor, concrete strength + 15MPa), strength reduction factor of 1.0

• Hope to add a further analysis type in the future to reflect a probable strength analysis (required by guidelines in NZ to assess existing structures)

To achieve the overstrength analysis, the code simply copies the concrete section and updates the material to the modified materials reflecting the likely upper limit material strengths.

There were a few minor changes I made outside of the NZS3101 design class to correct docs, or add other methods required.

Open to any comments you might have on progress!

[Agent6-6-6]

Hi @robbievanleeuwen
I've finished all of the NZS3101 class code to a working state, implementing all of the features I wanted to add to enable the three types of analysis typically required by NZS3101:2006.

I've also added the ability to assess sections to the NZSEE C5 assessment guidelines (which is basically the bible we need to follow here if assessing existing buildings), there are two additional types of analysis that fit under this umbrella. I wasn't going to do that before the first merge, but isolating at the moment with covid and got nothing better to do!

• The only feature I'll look to add once it is merged as a separate option would for singly reinforced walls (phi = 0.75 for designing these in plane).
• Also need to review the default parameters for labels on M/N curve, there is a # TODO somewhere that I need to tidy up.
• Create some example calculations similar to your AS3600 examples

Still need to do check of built docs and do some basic independent verification of capacities and so forth.

Thanks.

One thing I've never had anything to do with before is doing the tests, that's the only check failing at the moment. Any advice/help on this would be appreciated?

[robbievanleeuwen]

Hi @Agent6-6-6, thanks for all your hard work on this! I'll review this weekend :)

[Agent6-6-6]

@robbievanleeuwen
Docs should be updated now with c09bdd5, they built fine locally after some tweaking/rearranging. But wasn't sure if there is a way to view the built docs on *github.io as part of the pull request tests like you could for readthedocs docs to make sure they have all built fine remotely?

I took the liberty of updating some of the AS3600 docs to be consistent with respect to the material assumptions being in a note.

Essentially the code is ready for review, but do need to do the formal verification of capacities, etc. Hopefully by weekend.

Couple of things to consider:-

• There is quite a long docstring for the analysis_type parameter, rather than repeat it 4 times should it just be in the capacity_reduction_factor method (since this occurs first) and refer back to that from the other three locations with a link? Will get rid of a lot of repeated code in the 'DesignCode' class?

• In the 'init' class, the docstring Inits the NZS3101 class. is turning up in the docs, note you have the same in the AS3600. Should this just be removed since there are no variables required for the docs? I believe you can filter out certain entries using Sphinx and some custom code in conf.py based on a quick web search (as an alternative approach).

• I note in the AS3600 moment_interaction_diagram method that you have no means of feeding a theta to it, you're stuck just doing the analysis at theta = 0. Also I wonder if more of the parameters for altering the look of the resultant code design interaction curves should be exposed the user, i.e. missing means of feeding in theta, n_points, labels, etc?

• Should there be a general warning based on the analysis currently being done on constant neutral axis angle vs load angle, and that it may give false impression of demand/capacity when operating close to the curve?

• Need some guidance on tests

• Need to create an example

Any feedback or changes are welcome.

[Agent6-6-6]

@robbievanleeuwen having had a little time myself now to look through the examples and how the design code pages are linked together/arranged. I'm just wondering how you want to organise these if more codes are added. In particular these two pages:-
https://robbievanleeuwen.github.io/concrete-properties/rst/design_codes.html
&
https://robbievanleeuwen.github.io/concrete-properties/notebooks/design_codes.html

I'm wondering if the first link should have the AS3600 & NZS3101 stuff moved on a subpage linked from this page?? Or is the preference just one long list of codes added one after the other?

On the second link, were you thinking something similar but organised into subpages with some linked subpages with the actual examples from different codes as they are implemented?? I think that would be the best option to keep things concise?

Anyway if you do get some time this weekend to review, have a think about what your preference might be. If separating them out into separate pages is that something you're able to or prefer to do to prepare for the NZS3101 design code page and example, creating a placeholder page essentially?

Then I can likely replicate your AS3600 page as a starting point for completing that part of the docs?

[Agent6-6-6]

@robbievanleeuwen, marked ready for review. Let me know if you have any comments. Otherwise, merge if happy!

[robbievanleeuwen]

@Agent6-6-6 thanks this looks great! My kid's got chickenpox at the moment, but hopefully I'll have a chance to have a look at this at the end of this week/early next week! Love your work!

[robbievanleeuwen]

Hi @Agent6-6-6, finally got the time to go through the entire PR. I started writing a review then realised it would be easiest if I commit some minor changes as a lot of it was styling, linting things etc. We can revert anything you don't agree with, but here's the gist:

• I use the VS code import organiser, which just keeps things neat and consistent
• Docstrings with backslashes can cause issues if they aren't raw strings, i.e. add r before the """
• Ensuring all methods have a return type
• Ensuring all variables are bound before returning (these probably don't need a raises ValueError in the docstring as they are generally private methods)
• Type checking fixes

Other than the above everything looks really fantastic! Thank you so much for taking the time to make the code readable and understandable, and the documentation is amazing!

I have a few comments regarding the ModifiedMander which I'll address with some code comments. Other than that, I think we're good to go!

[Agent6-6-6]

@robbievanleeuwen, yeah go for it.

Agree with items 1 to 3, thought I had import organiser installed but apparently not.

Item 4 no idea what you're really talking about 😊 similar for 5. So feel free to amend as required and I'll have a look see if it triggers any other minor amendments or tidying up.

[robbievanleeuwen]

Item 4 no idea what you're really talking about 😊 similar for 5. So feel free to amend as required and I'll have a look see if it triggers any other minor amendments or tidying up.

Sorry wasn't super clear! I've started using Pylance to catch some potential code loopholes.

Item 4 - for example in NZS3101.assign_analysis_section(), it's possible to go through the if statement without assigning a value to analysis_section. Thus return analysis_section will raise a NameError as analysis_section won't be defined. While you are probably handling this input elsewhere, it's good practice to catch this.

Item 5 - I've added a few # type: ignore comments where the type checker (Pylance) isn't quite understanding what's happening or other packages haven't got their types strictly defined.

None of this changes any of your implementation, more for removing the red squiggly lines from my code editor 😆

[Agent6-6-6]

For item 4, I think (at least some of) the input was checked/validated earlier in the code so by the time you get to that location there is no way you could have a wrong value. So returning a value error seemed redundant. But appreciate if you called that method by itself with a wrong input it would fail. But I really need to refresh my mind what I did there as I could be wrong in the way I think I addressed this. But I guess I'll just need to raise new tests against these new lines to get back to full coverage to close it out if it makes code more complete with no loopholes.

Will review the comments in ModifiedMander class. Cannot recall why I just kept it as numpy array throughout, will review if it works as a list.

[Agent6-6-6]

Hi @robbievanleeuwen

For assign_analysis_section() added a new test for the additional else statement you added to check for an incorrect analysis_type. Note though this incorrect analysis_type is checked earlier in the code when it is first encountered within the capacity_reduction_factor() method so it is effectively redundant in the assign_analysis_section() method if you're using the NZS3101 class as intended. But added for completeness. See commit d3c4504.

Started working on the others but I got to thinking that the same redundant check looks like it applies to all the other else statements added. At the point where the methods execute there is no way that the wrong value could be assigned as it has been checked earlier in the code execution at the first instance it was required in other methods and was not being checked at each and every subsequent instance it was used within subsequent methods.

I can add tests to cover the additional else statements and amend some of the value error messages to be the same as the first location where it is checked to be consistent throughout if it is really required, but it would only ever be executed in the tests so it seems like adding code for no real purpose.

You may have missed one else as well in concrete_capacity() method for after elif self.section_type.lower() in ["wall", "wall_sr_s", "wall_sr_m"]:? But this still is a redundant check that would never be executed except for within a test as it is checked earlier in the assign_concrete_section() method the first time it is encountered for example.

Please review given the redundant nature of these if you still actually want them included (I mean no harm in having them, but they are not required if you're using the NZS3101 class as intended by design. But I can see if someone is accessing a method outside of the Class you'd want to catch the failures, but it was never written with this use case in mind as everything you need is intended to be in the NZS3101 class, from creating section to analysing it. But I guess until people start using it hard to say how people might use it and if this would therefore throw an error).

Note there were tests previously to capture the first instance of a wrong input for analysis_type & section_type for example, showing that these subsequent methods would never be executed with the wrong inputs. There was no way you could create a section consistent with the NZS3101 class without utilising the SteelbarNZ class for the reinforcement.

Some tests to hit these redundant else statements are a bit of a pain as multiple methods strung together would require the same else statement (max_comp_strength() calls concrete_capacity() for example both would require testing of section_type, both of which would require an else statement to capture the redundant incorrect input for completeness).

Anyway just wanted to double-check if this is really required prior to going to the trouble of adding additional else statements which are never hit by the actual code execution and then writing specific tests to hit these lines of code for the purposes of only the code coverage percentage? As that seems a bit redundant to me writing code/tests for something that is never able to be executed in a normal use case due to the preceding code by design checking the same thing.

Let me know your thoughts.

[robbievanleeuwen]

Hi @Agent6-6-6, I am happy with the code as is, thanks for these changes. We definitely don't need to aim for 100% test coverage, I don't believe in writing tests after the fact just to get full coverage, although I am definitely guilty of not writing enough tests myself! Having said that, your ~97% coverage is commendable!

I have built the docs locally and everything looks fantastic. I just have to do a few fixes with the new shapely release in sectionproperties, hence the <2 requirement for now. If you are happy with the code in it's current state I will merge and create a new release!

[Agent6-6-6]

There are a couple of the statements you added that I'll just update to be similar valueerror message to other locations to be consistent. But apart from that I didn't have anything else. I'll do this later today when I'm free to spend some time on it.

[Agent6-6-6]

Hi @robbievanleeuwen,

Nearly complete now with the following changes being committed in cbef839 to tidy up the loose ends:-

• Changed one of the additional ValueError checks from max_comp_strength() to concrete_capacity() as the concrete_capacity() is called from max_comp_strength(), so this is the initial location that should throw the ValueError if either method is called independently. Added a test for this.
• Added tests for the non SteelBarNZ material ValueError, as realised it is quite possible that you could assemble a section and assign it to the NZS3101 class as concrete_section by bypassing the create_concrete_section() method and end up with another steel material that is not compatible with the NZS3101 class requirements. However, I'm not hitting these lines of code with the test which I don't yet understand so need to check further....
• Updated docstrings for completeness to add the :raises ValueError: explanations where the new checks were located.

Just need to look into the second point further then it should be ready to merge.

[Agent6-6-6]

@robbievanleeuwen, all good to merge now with a1656b3 addressing point above.