Skip to content

Comments

Deriving RollPass and ThreeRollPass from Common Base#244

Merged
axtimhaus merged 6 commits intomainfrom
roll_pass_base
Aug 23, 2024
Merged

Deriving RollPass and ThreeRollPass from Common Base#244
axtimhaus merged 6 commits intomainfrom
roll_pass_base

Conversation

@axtimhaus
Copy link
Member

@axtimhaus axtimhaus commented Aug 22, 2024

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

Copy link
Member

@ChRen95 ChRen95 left a comment

Choose a reason for hiding this comment

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

So more or less moved the contents from RollPass to BaseRollPass.
Generally I think it's a good idea and will bring more structure to the codebase.
Also it makes deriving Classes easier.

@RichardPfr
Copy link
Contributor

RichardPfr commented Aug 23, 2024

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

def contact_contour_lines(self: DeformationUnit.Profile):
if isinstance(self.unit, RollPass):
rp = self.unit

Or am i understanding this wrong?

@ChRen95
Copy link
Member

ChRen95 commented Aug 23, 2024

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

def contact_contour_lines(self: DeformationUnit.Profile):
if isinstance(self.unit, RollPass):
rp = self.unit

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.

  • test_solve3 -> Seems to be an error in report due to the new Class
  • tests/roll_pass/test_roll_pass_profile_locations_disks.py -> I don't understand why this error never appeared before since hook and self don't match

@RichardPfr
Copy link
Contributor

RichardPfr commented Aug 23, 2024

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
Copy link
Member

ChRen95 commented Aug 23, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

rebase

@axtimhaus axtimhaus merged commit 1b5cbba into main Aug 23, 2024
@axtimhaus axtimhaus deleted the roll_pass_base branch August 23, 2024 09:55
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