Structure_Engine: Transform methods added for structural elements

[pawelbaran]

Issues addressed by this PR

Contributes to resolving #2322

Test files

On SharePoint. I have disabled the test of FEMeshes because for some weird reason they slow GH down badly even if a completely unrelated part of the script gets modified - I could not come up with an explanation for that, maybe the reviewer will have an idea 😃 However, this is outside of the scope of this PR, because the above issue does not affect the correctness of the results, so please do not be afraid of enabling the disabled part.

Changelog

• Transform methods added for Node, Bar, RigidLink, Edge, Opening, Panel and FEMesh

Additional comments

I believe this is the most problematic/sensitive of all transform-related PRs so far, so probably worth @IsakNaslundBh's time for a proper review. I hope the provided test files will make the process a bit less painful.

[pawelbaran]

Tolerance and UT added @LMarkowski @IsakNaslundBh, the PR is ready to review

[IsakNaslundBh]

Generally working great!

Just a comment on passing on the tolerances

[pawelbaran]

I have committed suggestions made by @IsakNaslundBh plus updated the GH script as well as the UTs including the extra test content provided by @IsakNaslundBh too. Should be ready for re-review now 👍

[IsakNaslundBh]

Another quick one, see below

[LMarkowski]

Works and looks good to me! As in the physical added the erroneous cases to the UT.

[LMarkowski]

@BHoMBot check unit tests when you'll be upside up again.

[LMarkowski]

@BHoMBot check unit

[bhombot-ci]

@LMarkowski to confirm, check-unit-tests task is now queued.

[LMarkowski]

One more thought that I forgot to include in the review. The test only includes a validation for Panels and is missing Edges and Openings. Do the two latter methods occur outside the first one? If not, why keep them public? And if they are, is test for only Panel enough for us? For me - it is. But in the future I can imagine those two being 1 missing percent while doing analytics for unit test coverage across the engine.
@rolyhudson @al-fisher what are your thoughts on that foundational-side?

(Edit: Same situation with CeilingTile in #2352)

[pawelbaran]

@LMarkowski the methods for Edge and Opening are public to reflect the general philosophy of the BHoM (if something does not strongly need to be private, keep it public). I have not added type-specific tests for them (same as for CeilingTile) because they are being tested in the Panel transforms (and Ceiling, respectively).

Not sure if adding those tests as separate entities would bring any value - but can do, if you like, this should not be much of an effort.

[LMarkowski]

@pawelbaran as said I think test for panels is enough for me as well validation-wise. My only concern are missing UTs. They could be done even by transforming exploded panels. The question is if it's worth it.

[FraserGreenroyd]

@pawelbaran as said I think test for panels is enough for me as well validation-wise. My only concern are missing UTs. They could be done even by transforming exploded panels. The question is if it's worth it.

It's always worth it, but it might not be valid to bring it in in this PR. The question is more accurately as whether it's worth it for this PR, not worth it overall.

[pawelbaran]

If it is worth it overall, I will add the UT's in right now - exploding the panels and kicking them in will take a minute, literally.

[LMarkowski]

The question is more accurately as whether it's worth it for this PR, not worth it overall.

@FraserGreenroyd Well, having some time put in adding missing tests I'd say we should enforce adding the UT every time we add a new public method. Reduce backlog.

(What about private ones though 🤔)

[FraserGreenroyd]

@FraserGreenroyd Well, having some time put in adding missing tests I'd say we should enforce adding the UT every time we add a new public method. Reduce backlog.

Yup, agreed. That's why Environments/MEP are enforcing new Unit Tests on PRs (@pawelbaran found that when I requested changes on his MEP version of this PR solely for a missing unit test!). I believe structures are doing the same but @IsakNaslundBh can confirm that. I definitely recommend it though!

(What about private ones though 🤔)

Private methods don't get exposed but are used by public methods to aid calculations, so the unit tests of public methods will be good enough (for now, we might change that later mind).

[pawelbaran]

Oke, I have added the missing UTs and updated the GH script.

[pawelbaran]

@BHoMBot check all

[bhombot-ci]

@pawelbaran to confirm, check-code-compliance, check-documentation-compliance, check-project-compliance, check-branch-compliance, check-dataset-compliance, if applicable check-copyright-compliance, check-serialisation, check-versioning, check-unit-tests, check-core, check-installer, and check-ready-to-merge tasks are now queued.

[pawelbaran]

/azp run BHoM_Engine.CheckInstaller

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[LMarkowski]

Now it's perfect! Thank you.
RTM as soon as we have an approval from @BHoMBot

[pawelbaran]

RTM as soon as we have an approval from @BHoMBot

You mean never?

[bhombot-ci]

Please be advised that the check with reference 2039032949 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 290 additional annotations waiting, made up of 290 errors and 0 warnings.

[pawelbaran]

@BHoMBot you fool

[bhombot-ci]

@pawelbaran sorry, I didn't understand.
Was that comment an instruction for me? If so, could you state again what check you would like me to do?
For example, if you would like me to fix the CSProject file(s) comment:

"@BHoMBot fix project file"

[pawelbaran]

@BHoMBot check installer

[bhombot-ci]

@pawelbaran to confirm, check-installer task is now queued.

[FraserGreenroyd]

You mean never?

https://media.giphy.com/media/i0xGuo4o5PutVo0imJ/giphy.gif

[FraserGreenroyd]

Ok, bot issues aside, this is now ready for @IsakNaslundBh to give a decision from his changes requested and go for merge at that point 😄

[IsakNaslundBh]

Have not tested after the changes, but trust @LMarkowski s test!

Issues I have raised have been resolved!

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

@FraserGreenroyd to confirm, the task for checking if this Pull Request is ready to merge is now queued.