A more appropriate way to define moon_radius constants

[ahmedhosssam]

Hi,
In sunpy/coordinates/sun.py in eclipse_amount(), I think that we could encapsulate the constants in a class K and initialize the value of k with K.IAU as a default, and then check if moon_radius is neither IAU nor minimum to throw an error.
I think the code is more readable now.

Please let me know if there are any improvements needed.

[nabobalis]

Can you explain why the class is better?

I guess I am left a bit confused as how it improves the code.

[ahmedhosssam]

I think it's more maintainable, as it makes it clear that these values are related and belong to a certain context.
Also it makes it easier to use the same value somewhere else in the code (if we want to), especially because it's a floating-point number. So it's better to encapsulate it.

[ayshih]

If you want to go this route, it should be a dictionary rather than a class. For example:

radius_options = {
    'IAU': 0.2725076,
    'minimum': 0.272281
}
try:
    R_moon = radius_options[moon_radius] * R_earth
except KeyError:
    raise ValueError("The supported values for `moon_radius` are: " + ", ".join(radius_options.keys()))

[nabobalis]

Albert, are these values not defined in astropy? And if not, should we upstream them there?

Or even as sunpy constants?

[ayshih]

Hence the TODO that I wrote: # TODO: Find somewhere more appropriate to define these constants

The constants are not defined in astropy nor does it make sense to add them there. It's not clear it makes sense to add them to our constants. It's academic until some other code wants to use these constants.

[nabobalis]

Hence the TODO that I wrote: # TODO: Find somewhere more appropriate to define these constants

The constants are not defined in astropy nor does it make sense to add them there. It's not clear it makes sense to add them to our constants. It's academic until some other code wants to use these constants.

If we are going through the effort to tidy up how we present these numbers in the function, I do not see why we should go with the slight extra effort to define them formally as constants.

[ayshih]

It's not code effort, but rather design effort. These are constants that make sense only for solar eclipses, hence way too specialized to put into astropy. Our existing constants are specific to the Sun – we have sunpy.sun.constants, not sunpy.constants – so we'd need to create a completely new place for Moon constants to go.

[nabobalis]

Sounds good to me, let's do it.

Could it be sunpy.constants.moon and sunpy.constants.sun?

[wtbarnes]

I think we should take a step back and ask ourselves if there is anywhere, in our existing codebase, besides this function, where these values would be useful. If the answer is no, I don't see the point in trying to expose them in some more formal way at this point.

[nabobalis]

In that case, the dict that Albert posted should be the method we use instead of a class.

[nabobalis]

Thanks for the PR @ahmedhosssam