pyroll-project / pyroll-core

PyRoll rolling simulation framework - core library.
https://pyroll.readthedocs.io
BSD 3-Clause "New" or "Revised" License
12 stars 7 forks source link

Deriving RollPass and ThreeRollPass from Common Base #244

Closed axtimhaus closed 3 weeks ago

axtimhaus commented 3 weeks ago

As an idea to solve the issues that rose in #241

This is a breaking change, so at least new minor version. For example the report breaks with this PR, because there is a check for RollPass only, which yields now results in 3RP only sequences.

This would also simplify future intro of multiple rolls as in #104

RichardPfr commented 3 weeks ago

I believe there are still some places where RollPass needs to be changed to BaseRollPass. One example i found: https://github.com/pyroll-project/pyroll-core/blob/77e9edab877bc7b85434e66ae5b65f71795041af/pyroll/core/roll_pass/hookimpls/deformation_unit.py#L94-L96

Or am i understanding this wrong?

ChRen95 commented 3 weeks ago

I believe there are still some places where RollPass needs to be changed to BaseRollPass. One example i found:

https://github.com/pyroll-project/pyroll-core/blob/77e9edab877bc7b85434e66ae5b65f71795041af/pyroll/core/roll_pass/hookimpls/deformation_unit.py#L94-L96

Or am i understanding this wrong?

So you don't have to change all the references from RollPass to BaseRollPass as every RollPass is also a BaseRollPass. In the case that you mentioned it is no problem. I just noticed, that when running the tests 2 fail to converge.

RichardPfr commented 3 weeks ago

Yes but previously this function was also able to be used by ThreeRollPasses, what is now no longer possible. So from my understanding these general functions should all specify BaseRollPass so that other more specific classes can use them aswell.

PS: I also use this function in the zouhar-package for ThreeRollPasses which is where i noticed that for example.

ChRen95 commented 3 weeks ago

Yes but previously this function was also able to be used by ThreeRollPasses, what is now no longer possible. So from my understanding these general functions should all specify BaseRollPass so that other more specific classes can use them aswell.

PS: I also use this function in the zouhar-package for ThreeRollPasses which is where i noticed that for example.

Your right, I thought the hirachy was still BaseRollPass -> RollPass -> ThreeRollPass but now is BaseRollPass -> RollPass, BaseRollPass -> ThreeRollPass. So you can adjust those and commit.

axtimhaus commented 3 weeks ago

test_solve3 -> Seems to be an error in report due to the new Class

This is why I said

For example the report breaks with this PR, because there is a check for RollPass only, which yields now results in 3RP only sequences.

@RichardPfr you are right with this hook. But this impl has nevertheless to be implemented on BaseRollPass, and not on DeformationUnit. So then the isinstance check is not necessary

axtimhaus commented 3 weeks ago

The wrong place of this hook impl made me missing it.

I fixed, moved and simplified it.

@BaseRollPass.Profile.contact_lines
def contact_contour_lines(self: BaseRollPass.Profile):
    rp = self.roll_pass
    return [linemerge(cl.intersection(self.cross_section.exterior)) for cl in rp.contour_lines]
axtimhaus commented 3 weeks ago

rebase