uxarray.conventions provides names of dimensions and descriptions of attributes. These are being imported by some parts of the code, but ignored by other parts of the code.
Example:
uxarray.conventions.ugrid defines FACE_DIM = "n_face". Other parts of the code vary in whether they utilize "n_face" directly as a string or utilize FACE_DIM instead. In uxarray.grid.coordinates there is code like xr.DataArray(..., dims=[FACE_DIM], ...). Meanwhile, in uxarray.dataarray there is code like cls.from_xarray(da, uxgrid, {face_dim: "n_face"}).
Because FACE_DIM is being used in some places, I initially got the impression that I need to use it instead of writing "n_face". It also made me think maybe the intention is that one day an adventurous user might expect to be able to do something like import uxarray.conventions as uc; uc.FACE_DIM = "custom_facedim" and have all "n_face" occurrences replaced by "custom_facedim" in results. However, because "n_face" is being used in some places, this would not actually work; it would likely just lead to a variety of confusing crashes.
Proposed change:
I would propose to swap to using "n_face" everywhere throughout the code. It would make the code easier to read quickly (e.g., I wouldn't need to know FACE_DIM=="n_face" to understand the code from uxarray.grid.coordinates anymore), and my guess is that we don't actually intend to support renaming of strings from the established conventions.
If there is a compelling reason to instead keep some occurrences as FACE_DIM and some as "n_face", then it would be very helpful to document that reason via comments somewhere in the uxarray.conventions subpackage.
I used FACE_DIM and "n_face" here as an example, but this logic should be applied to all the other conventions as well. For each convention, it should either be used consistently everywhere throughout the code, or nowhere throughout the code, or documented with a comment to justify the reason for mixed usage.
This change would be internal only, and should not affect any users, unless they were previously attempting to rename the values of conventions.
uxarray.conventions provides names of dimensions and descriptions of attributes. These are being imported by some parts of the code, but ignored by other parts of the code.
Example:
uxarray.conventions.ugrid defines
FACE_DIM = "n_face". Other parts of the code vary in whether they utilize "n_face" directly as a string or utilize FACE_DIM instead. In uxarray.grid.coordinates there is code likexr.DataArray(..., dims=[FACE_DIM], ...). Meanwhile, in uxarray.dataarray there is code likecls.from_xarray(da, uxgrid, {face_dim: "n_face"}).Because FACE_DIM is being used in some places, I initially got the impression that I need to use it instead of writing "n_face". It also made me think maybe the intention is that one day an adventurous user might expect to be able to do something like
import uxarray.conventions as uc; uc.FACE_DIM = "custom_facedim"and have all "n_face" occurrences replaced by "custom_facedim" in results. However, because "n_face" is being used in some places, this would not actually work; it would likely just lead to a variety of confusing crashes.Proposed change:
I would propose to swap to using "n_face" everywhere throughout the code. It would make the code easier to read quickly (e.g., I wouldn't need to know FACE_DIM=="n_face" to understand the code from
uxarray.grid.coordinatesanymore), and my guess is that we don't actually intend to support renaming of strings from the established conventions.If there is a compelling reason to instead keep some occurrences as FACE_DIM and some as "n_face", then it would be very helpful to document that reason via comments somewhere in the uxarray.conventions subpackage.
I used FACE_DIM and "n_face" here as an example, but this logic should be applied to all the other conventions as well. For each convention, it should either be used consistently everywhere throughout the code, or nowhere throughout the code, or documented with a comment to justify the reason for mixed usage.
This change would be internal only, and should not affect any users, unless they were previously attempting to rename the values of conventions.