Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made Disk and SplineRoundDisk robust to transformations #66

Merged
merged 15 commits into from
Jan 9, 2025

Conversation

LasseHolch
Copy link
Contributor

Disks

  • Corrected diagonal_ratio
  • Redefined spline curves, such they are stable, when core points are moved.

SplineRoundDisk

  • Rebased the on top of Disks, such they have the same quad_map
  • Redefined spline curves, such they are stable, when core points are moved.

Annulus

  • added sweep angle alowing for half rings

Cylinder:

  • created Half and Quater Cylinders, where symmetri patches can be named easily

@FranzBangar
Copy link
Member

Excellent work, I'll take a look ASAP

@FranzBangar
Copy link
Member

I tried to merge your changes but there are some issues (failing tests, type checking, etc.). Do you have your IDE prepared as instructed in https://github.com/damogranlabs/classy_blocks/blob/development/CONTRIBUTING.md? Pre-commit hook should run tests, analyze typing, sort imports, format code, ... Let me know if you need any help setting that up.

It seems your code contains a lot of lines like (self.side_1 + r_1 * np.cos(np.pi / 4)) * self.u_1, is there a way to un-repeat those? For instance, you could create a specialized parametric Curve object that would calculate that and re-use it in all round sketches. I can't tell you exactly what to do, I didn't have time to thoroughly check what your code does. I'd gladly help but I've been terribly busy last few weeks and it will stay so for the next few...

@LasseHolch
Copy link
Contributor Author

LasseHolch commented Jan 2, 2025 via email

@FranzBangar
Copy link
Member

Okay, if you're not in a hurry, you can just commit your changes on your local branch and I'll pull them and see what I can do. First I have to finish my W.I.P. though - but it looks promising so I might even get it done! In a while, of course. :)

@LasseHolch
Copy link
Contributor Author

LasseHolch commented Jan 9, 2025 via email

@FranzBangar FranzBangar merged commit 8f29231 into damogranlabs:development Jan 9, 2025
@FranzBangar
Copy link
Member

I modified your code a bit and merged your PR.
Here's what I did:

  • Formatting: there were some single quotes in examples; double quotes are default and should be replaced on save or commit (depends on your IDE). Since your other files are consistent I guess those examples were saved before you set your environment up.
  • Formatting: import sorting: the same as above but could be that isort/black/whatever tool gets an update, changes style then ruff whines about it. If that's the case, a simple pip update ... will do.
  • You defined an abstract orig_point attribute, then handled its transforms separately. That's some duplicated code and a high risk of errors (some tests were failing already). I put that abstract attribute into __init__ as a Point, then added that to the parts property. Now ElementBase that handles transforms of all parts (of sketch, shape, ...) also transforms that Point. The origo is now a property that simply exposes that Point's position. No more duplicate code!
  • There was a problem with DiskBase.core_spline method; you created SplineRound.core_spline with different arguments (type checker shouldn't let you do that though!). I renamed both so that those are now separate methods (which actually are) but I'm not sure I did everything correctly. Please take a look at combined_example, I suspect there's still a problem somewhere.
  • I'll create a separate example for a QuarterCylinder with symmetry planes.

Of course you won't have time on your parental leave, I've just been on one and wrote only somewhere around zero lines. But if you - by some weird coincidence - do find yourself programming, I strongly recommend you write tests for your splined shapes. If you cover your code with tests, I'll be able to refactor it with greater confidence I don't mess your code up.

pip install coverage
coverage run -m unittest discover tests
coverage html --skip-covered
<your_browser> htmlcov/index.html (focus on your source files)

I merged your updates into development branch but I don't think I'll be able to push all that into master just yet. The development branch is currently terribly messy with semi-finished features I still have to finish and that is not ready for the master yet. Is this a problem? Can you pip install directly from development branch?

Anyway, thank you for your contribution!

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.

2 participants