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

Use of <source class>.to_<target class> methods #155

Open
gumyr opened this issue Mar 2, 2023 · 8 comments
Open

Use of <source class>.to_<target class> methods #155

gumyr opened this issue Mar 2, 2023 · 8 comments
Labels
architecture review Discussion related to architecture changes prior to 1st release

Comments

@gumyr
Copy link
Owner

gumyr commented Mar 2, 2023

There are many methods that convert from one class to another - for example Vector.to_vertex(). These methods don't seem as pythonic as using the target class' __init__ method to do the conversion - e.g. Vertex(v:Vector). They also couple the classes together which makes separation into multiple modules impossible (or at least very difficult).

I'm proposing converting all of the to_class methods over to class(). Is there any reason not to do this?

@gumyr gumyr added the architecture review Discussion related to architecture changes prior to 1st release label Mar 2, 2023
@gumyr gumyr added this to the Release 1.0.0 milestone Mar 2, 2023
@Jojain
Copy link
Contributor

Jojain commented Mar 2, 2023

I propose this :
Splitting the topo class and the geom classes in 2 modules :

  • topology.py -> Shape, Faces, Vertex,etc with potentially new classes ( Section could subclass Compound and represents a collection of planar faces in the same plane for example)
  • geometry.py -> Vector, Plane, Axis and stuff. I propose to add a Point class. It make more sense to ask a Vertex to give it's point when asked for it's geometry rather than a Vector a vector has a length which a Point of Vertex does not.

@gumyr
Copy link
Owner Author

gumyr commented Mar 2, 2023

Although I haven't pushed it yet, I have have two modules: geometry.py and topology.py. All the tests and examples pass but the documentation will need to be looked at. There was only a minor bit of messiness as Planes can be constructed from Faces which needed to be re-worked (with the same functionality).

@jdegenstein
Copy link
Collaborator

wow, that is awesome progress!

@gumyr
Copy link
Owner Author

gumyr commented Mar 3, 2023

The restructuring to geometry.py and topology.py is complete. Onto elimination of the other .to_ methods...

@jdegenstein
Copy link
Collaborator

Taking a quick look at the to_ methods, geometry.py has these (line no's on left):

234 | def to_tuple(self) -> tuple[float, float, float]:

417 | def to_pnt(self) -> gp_Pnt:
 
421 | def to_dir(self) -> gp_Dir:
 
584 | def to_plane(self) -> Plane:
 
845 | def to_align_offset(self, align: Tuple[float, float]) -> Tuple[float, float]:
 
949 | def to_tuple(self) -> Tuple[float, float, float, float]:
 
1282 | def to_axis(self) -> Axis:
 
1286 | def to_tuple(self) -> tuple[tuple[float, float, float], tuple[float, float, float]]:
 
2088 | def to_gp_ax2(self) -> gp_Ax2:
 
2136 | def to_local_coords(self, obj: Union[VectorLike, Any, BoundBox]):

topology.py has these (line no's on left):

2847 |  def to_splines(

2880 | def to_vtk_poly_data(
 
2933 | def to_arcs(self, tolerance: float = 1e-3) -> Face:
 
4270 | def to_wire(self) -> Wire:
 
5001 | def to_axis(self) -> Axis:
 
6554 | def to_tuple(self) -> tuple[float, float, float]:
 
6682 | def to_wire(self) -> Wire:

Lastly jupyter_tools.py has def to_vtkpoly_string(...

Anything I can do to help trim these down / refactor?

@gumyr
Copy link
Owner Author

gumyr commented Dec 12, 2023

How does this sound:

  • rename the ones that generate OCP objects (gp_Pnt, gp_Dir, and gp_Ax2) to add an underscore as these are somewhat private/internal methods

  • Axis.to_plane be replaced by adding Axis as one of the initializers of the Plane class which shouldn't be too difficult as they are both geometry classes.

  • BoundBox.to_align_offset isn't really changing classes so I don't think it's a problem

  • Color.to_tuple() again isn't really the same but maybe it should be replaced with red, green, blue, and alpha properties that return the values returned by to_tuple(). If so, to_tuple() could be removed unless you think there is a good reason to keep it.

  • Location.to_axis() could be replaced by adding Location as an input option to Axis.

  • Location.to_tuple() is an interesting one. There are position and orientation properties that could take on this role but the orientation part uses: GetEulerAngles(gp_EulerSequence.gp_Intrinsic_XYZ). Is more flexibility required here now that there are more options?

  • Plane.to_local_coords is a totally different thing

  • Shape.to_splines isn't in this category

  • Shape.to_vtk_poly_data is a bit of a mystery to me - do any of the viewer use this method?

  • Shape.to_arcs seems like it should be a Face method not a Shape method. Can't really imagine a use for this.

The sub-classes of Shape don't have init methods and should if the to_wire methods are to be replaced. A bit of work though.

@snoyer
Copy link
Contributor

snoyer commented Dec 13, 2023

FWIW the built-in tuple() initializer takes an iterable so technically you could get rid of .to_tuple() by overriding __iter__...

@dataclass
class A:
    x: float
    y: float
    z: float

    def to_tuple(self):
        return self.x, self.y, self.z

    def __iter__(self):
        return iter((self.x, self.y, self.z))


a = A(1, 2, 3)
assert a.to_tuple() == tuple(a)

__iter__ also allows unpacking

x,y,z = a
assert tuple(a) == (x,y,z)

I won't risk claiming whether this is the way to go or not though

@gumyr
Copy link
Owner Author

gumyr commented Dec 14, 2023

Both Vector and Vertex are Iterables already, so:

print(tuple(Vertex(1, 2, 3)))
print(tuple(Vector(4, 5, 6)))

results in

(1.0, 2.0, 3.0)
(4.0, 5.0, 6.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture review Discussion related to architecture changes prior to 1st release
Projects
None yet
Development

No branches or pull requests

4 participants