Closed mhvwerts closed 5 months ago
Funny you mentioned it @mhvwerts. I was working on a software that switches between Cartesian and Cylindrical coordinates, with some strange BCs changing with time. I found myself looking at the source code a couple of times because I was not sure about what x and y means, mostly for xvalue
and yvalue
of FaceVariable
s. I think it is a good idea to find a simple solution for it. I had a vague idea for defining subclasses, but cannot test them before the mid-September. All in all, I must admit it is a very desirable feature to have.
Oh yeah, xvalue
and yvalue
of FaceVariables
, I forgot to mention those... I am experimenting with convection-diffusion in 2D cylindrical (typical Taylor dispersion experiment) as this is a relevant test case and it took some trial-and-error to set up the convective term (velocity component in z direction, magnitude dependent on r).
Let's give it time. IMO, implementing this feature should be done in one run, and probably best that I make sure that we have a decently working pytest
(#12) before such an undertaking as it may break stuff.
Absolutely agree. Let's keep it as an open issue. I usually do some python-therapy when I get to busy with writing tasks. If I come up with a good idea, I will write it here.
I just tried Co-pilot and it suggests using property decorators:
class MyClass:
def __init__(self, xvalue, yvalue):
self.xvalue = xvalue
self.yvalue = yvalue
@property
def rvalue(self):
return self.xvalue
@rvalue.setter
def rvalue(self, value):
self.xvalue = value
@property
def zvalue(self):
return self.yvalue
@zvalue.setter
def zvalue(self, value):
self.yvalue = value
The same thing can be done with a subclass. We also need to define subclasses of FaceVariable
for different coordinates and then use the decorators.
That's an interesting suggestion. It may be wise, perhaps necessary, to combine this with renaming the 'internal' x
, y
, z
to _x
, _y
, _z
(or x_
...). I see two reasons to do so:
z
becomes ambiguous: 'exterior/user' z
refers to 'internal' y
, but what happens with the 'internal' z
? (I am not sure if the internal z
exists in 2D cylindrical, I know it is of no use. The conflict will surely exist for 3D cylindrical.)I agree. They cannot be hidden from the user but the underscore can help. This z
being y
in cylindrical coordinate is one of those stupid thing I realized later like all other stupid things I have done in my life :-))
The re-labeling strategy might consist of a well-targeted and supervised find-and-replace of .x
.y
.z
by ._x
._y
._z
followed by fixing any errors that occur through adapting/providing subclasses or changing the user code. Do you think that that might work?
It will work but needs some work. I wish Python would let us define private attributes but it is not the case.
The underscore at the beginning of the attribute name should be enough a deterrent for the casual user not to touch these attributes.
PyFVTool systematically uses the labels
x
,y
andz
to represent the first, second resp. third coordinate, irrespective of the mesh geometry (Cartesian, cylindrical, spherical). This is important especially for internal code consistency and keeping the code base simple and free from Pythonic wizardry which may generate head-aches. Once used to it, this is OK, and one writes, e.g.,However, it would still be nice, one day, to consider the possibilities for adapting the programming interface to
cellcenters
andfacecenters
(andcreateMeshCylindrical
etc.) such that the user would only see and user
,z
(andtheta
,phi
) when working with cylindrical or spherical meshes. This would improve the readability of the user's code and even avoid some errors. Internally, PyFVTool would continue to use labelsx
,y
andz
, but this would be invisible to the user, and translated by the functions and methods for creating and accessing meshes.I have some vague ideas how this could be achieved in a relatively simple manner, but already wondering if @simulkade thinks if this is somewhere remotely feasible and desirable, and what others would think of such a new feature.