split off astronomical constants from units #1059
Conversation
|
There is no constant for metallicity. The closest would be 'solar metallicity', but even for that there isn't a generally accepted constant value. |
|
Yeah, and it's a fraction anyway, right? It just occurred to me that metallicity was at the bottom of the file, amidst degrees and grams and centimeters which are not discipline-specific. Anyway, something for later. Will you merge this? |
|
There's a remaining issue: names are used both for the constants and for the unit based on the constant. |
…etween units/constants.
I think the NIST_URL is no longer operational, but manual downloading and passing the filename works.
|
(Of course I'm violating that "rule" myself in a commit only minutes after posting this...) |
| pickled_km = pickle.dumps(km) | ||
| unpickled_km = pickle.loads(pickled_km) | ||
| self.assertEqual(1000, unpickled_km.value_in(m)) | ||
| kilometer = 1000 * m |
There was a problem hiding this comment.
renaming it here since 'km' is already defined... could probably just as easily use the pre-defined 'km' unit, but maybe there was a reason not to?
|
|
||
|
|
||
| au = 149597870691.0 | m | ||
| parsec = au / np.tan(np.pi / (180 * 60 * 60)) |
There was a problem hiding this comment.
Note that both 'au' and 'parsec' are also defined as units... Be careful, this may clash!
For Lsun etc below, note that here they only have one capital letter, the unit has LSun etc... This is to be more in line with Astropy etc, which only have one capital letter. Still, it may be better to be consistent within AMUSE?
There was a problem hiding this comment.
Wow, that's a really subtle problem actually. Having a unit and a constant with almost the same name but different capitalisation seems like asking for trouble, because how do you remember which is which? Perhaps the best solution is to have a single object that can be used in both ways? But that would probably require some major redesign....
There was a problem hiding this comment.
normally it wouldn't be a problem, since units would be used as units.(unitname) rather than being directly imported. But I don't exactly like it.
There was a problem hiding this comment.
Are constants that can also be units actually ever used as anything else than units?
There was a problem hiding this comment.
Probably not. Except for making the units.
There was a problem hiding this comment.
So... We could in principle make the names here (way) more descriptive, e.g.:
distance_parsec = ...
distance_au = ...
luminosity_sun = ...
radius_sun = ...
That would solve this issue. It's very verbose but since you're always using the unit form that should not be a problem.
There was a problem hiding this comment.
Well, they could still have the same names actually, as long as no one directly uses the amuse.units.amuse_2010.astronomical_constants module. It should only be used in the modules that define the units then, and we could even name it _astronomical_constants to mark it as private.
Then in the modules where the units are defined, we could import amuse.units.amuse_2010.astronomical_constants as ac and write ac.au and the like everywhere to clearly point out where they're coming from and that these are the constants and not the units. I think that's what I'd do.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 28 days if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
stale
rieder
added
feature request
and removed
stale
| def get_translator(self): | ||
| f = open(os.path.join(self.directory, 'translator.txt'), 'r') | ||
| f = open(os.path.join(self.directory, "translator.txt"), "r") |
There was a problem hiding this comment.
Please keep real changes and formatting changes from black in separate commits, can't see the forest for the trees here...
Astronomical constants have changed, this is part of an effort to make these easier updateable.
units.py should not contain changeable values anymore.