Draft: An equivalency for geometric conversions - #12847

In the above example I changed mass_length_time to schwartzchild_system. Looking at https://en.wikipedia.org/wiki/Geometrized_unit_system, there a many geometrized unit systems, e.g. Planck natural units, so
mass_length_time isn't specific enough to inform the user that the relationship is through Schwarzschild radius of a nonrotating uncharged black hole. I like the idea of adding new unit systems and the one in this PR would be a good addition amongst many: Planck, cosmology (8πG = 1), Galactic & extragalactic (rescaled SI), etc.

These functions don't rely on any arguments to the equivalency, so they can be defined outside the function. That way they can also be serialized by, e.g. pickle.
It's probably good to make them "private" functions

def _sol_mass_to_meters(mass):
    return mass * schwarzchild_factor.to_value(u.m)

Also, I think having variable names is easier to read, so

Also, using to_value

Good tests!

For Astropy code contributions we actually don't directly commit to CHANGES.rst, but instead use a system call towncrier to assemble the changelog.
See https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#add-a-changelog-entry
I see you added a changelog fragment already, so this can be removed.

Given that there are many types of geometrized unit systems, I think this should be renamed.

To add to @nstarman's comments, for equivalencies the functions should not use units internally, rather they should assume the input are values in one unit and produce output that are values in the other unit. Probably easiest to look more closely to the other examples in this file.

In fact, since all these conversions are just multiplications with numbers, there is no need to have any function: one should just give the right input and output units. E.g., for the mass one wants to use the mass that would correspond to 1 m:

In [7]: mass_length = [(u.Unit(u.kg*const.c.value**2/const.G.value), u.m)]

In [8]: (1*u.Msun).to(u.km, mass_length)
Out[8]: <Quantity 1.47662504 km>

Just glancing at this quickly... Maybe you've thought about this and this is the better implementation, but this feels like a case for Enum.

Just looked at the docs for Enum, seems like a similar approach. Both Enum and Dataclasses are part of the standard library.

Another case for Enum ?

Assert statements are stripped in certain python compilation modes. If you don't want this check run in "optimized mode", this is fine, otherwise use an if statement and manually raise the desired error

I wanted to take this opportunity to check if this is done in the CI pipeline.

I think with Enum you don't need to define a new instance each time.

I have not worked with Enum a lot. But let me know if this is a potential issue. I could, as well, move the instantiation outside the scope of the function at the module level.

I think all these equivalencies can be defined external to this function.
And @mhvk mentioned that these might not even require lambda functions. Not sure if that's still the case, but worth checking out!

It can be but in almost all other existing equivalencies, this is how it has been implemented.

For example:

• https://github.com/astropy/astropy/blob/main/astropy/units/equivalencies.py#L126-L137
• https://github.com/astropy/astropy/blob/main/astropy/units/equivalencies.py#L203-L237
• https://github.com/astropy/astropy/blob/main/astropy/units/equivalencies.py#L340-L364

So that might be a part of larger refactoring that needs to be set in a future milestone

OK I see that there is a stage where the tests are run in parallel: https://github.com/astropy/astropy/blob/main/.github/workflows/ci_workflows.yml#L150-L181 which is passing, so I believe there is no issue with serialization etc

My apologies actually. I see that this stage had passed in a previous pipeline, but in the most recent one it has failed. Please ignore my above comment

OK, the failure was caused by a typo in a docstring unrelated to what I had mentioned before.

I am guessing your concern is regarding the serialization of functions for multiprocessing. I believe the parallel mode for tox in the 32-bit and parallel stage would have caught that. But, like I mentioned, my implementation follows that of most other existing equivalencies. Maybe refactoring the callables could be an item for a later release if its an issue.

As @nstarman noted, there should be no need for the lambda functions.

Hi @mhvk based on the discussion we had earlier, I have changed the PR to include a few more physical types. I will mention in brief what all has been taken into account.

• Irreducible units mass, length, time can be converted into each other
• Composite units which are homogeneous like area i.e. length squared can be converted to mass squared etc
• The challenge is for heterogeneous composite units like density. Currently it works for kg/m^3 to 1/kg^2, 1/s^2 etc and the other which way. But not for kg/m^3 to g/s^3, or to 1/(m*s) etc.
  Having support for all of these is a nice to have but will need more work. But let me know what you think of this until now. I have tried adding tests for each of the conversion, but if something is missing let me know. Adding support for other physical types like pressure/force is not difficult if we restrict the support to only target unit with a single base - like pressure in SI to 1/(length or time or distance)**2, similar to mass density.

Also since the code has changed a lot since my initial PR, I am happy to rebase to a single commit if that helps. I have not yet taken care of the definition of the G = 1 vs. 8 pi G = 1, but its on my radar. Thanks

Comparisons are normally done like

assert_quantity_allclose(mass.to('kg', equivalencies=u.geometrized('mass'), comparison, atol=...)

see elsewhere in this file.

These particular tests do not actually seem to do anything interesting! You are converting something that is already a mass to a mass (and length to length, time to time) here, which will not even use the equivalency.

These tests are best split in 2, by pytest.mark.parametrizeing the function for u.m and u.s - as is, it will just check u.m, see that it is equivalent, and return True

As @nstarman noted, there should be no need for the lambda functions.

These functions duplicate what the unit machinery already does - you just need to define the equivalency with the units scaled the right way -- see my earlier comment.

there should be no need for the lambda functions.

Can you help me understand this better. As I mentioned before, there are several places, where this is done: #12847 (comment)

the third and fourth arguments of the tuple need to be callables?

The third and fourth arguments can be omitted, in which case effectively it is treated as a pass-through function. So, conceptually, for q.to(new_unit, equivalencies=[(unit1, unit2)]), the quantity will first be converted to unit1, then its unit is replaced by unit2, and finally it is converted to new_unit. Maybe easiest to look again at #12847 (comment) and play around with the example there in an interactive session.

This linked if-elseif statement could I think more logically be done with a dict that is keyed by physical_type (they are hashable, so that should work), i.e.,

return geometrized_equivalencies[physical_type]

To me it is slightly illogical to define this in terms of string units - I think conceptually it makes more sense to have a dict of equivalencies (see below), and here a dict of dict keyed by the units rather than there strings. But I think it can be done more directly by just defining a dict of equivalencies -- see main comment.