UnitfulAstro.jl icon indicating copy to clipboard operation
UnitfulAstro.jl copied to clipboard

feedback?

Open mweastwood opened this issue 7 years ago • 7 comments

Hi @ajkeller34

I think I said I would write this package several months ago. I've thrown together a rough draft this evening using UnitfulUS.jl as an example. I still need to do docs and tests, but I wanted to alert you to this repos existence.

  • Is it ok if I have Unitful in the name of the package? I wanted to make it clear that this package extends Unitful (and is therefore compatible with Unitful).
  • I usually like to use the GPLv3 license, but I've used the MIT license here so that the Unitful* line of packages have a consistent license
  • ajkeller34/Unitful.jl#41 would be really cool to have because astronomy has a bunch of extremely confusing logarithmic units

Any comments?

mweastwood avatar Jun 20 '17 06:06 mweastwood

I should also mention that I'd be happy to give you commit access to this repository.

mweastwood avatar Jun 20 '17 06:06 mweastwood

Great to see this! Fine with me if you use the Unitful name. I have begun to think about how I might implement logarithmic units but may bounce some ideas off you at some point. If you were to give me commit access I'd only use it in the event of Unitful syntax changes, for which I can also just submit PRs, so it's as you wish.

A few comments on the code:

  • You don't need to export @us_str, that was just a bit of cleverness for the UnitfulUS package so that I could do us"gal" as US gallon (as opposed to the imperial units which can be slightly different but similarly named).
  • I'm beginning to doubt if I should have advised people to put Unitful.register in __init__(). It's analogous to exporting your units, although the "namespace" in this case is just what you can type in the unit string macro. In a way, it's actually worse though, since unlike the import vs. using distinction, there's no way to control the behavior if the module is registered in __init__. They just automatically become available to the @u_str macro. The alternative would be to remove the Unitful.register line and just advise people they need to do that if they want to access your units via u"AU" rather UnitfulAstro.AU. Anyway, you can choose the behavior you prefer.

ajkeller34 avatar Jun 20 '17 15:06 ajkeller34

You don't need to export @us_str

Oops, I think that was a copy-paste mistake.

I'm beginning to doubt if I should have advised people to put Unitful.register in __init__()

Maybe if Unitful.register becomes a problem the solution is to put the units definitions into even smaller sub-modules so that you're not pulling in the kitchen sink every time you call using Unitful*?

There should also probably be some tests that units do not conflict with each other. For example parsecs (pc) would conflict with the speed-of-light unit (c) if you turned on SI-prefixes (pico-speed-of-light).

mweastwood avatar Jun 20 '17 17:06 mweastwood

Hi! Very interesting package indeed, you may also consider proposing the inclusion in JuliaAstro (and maybe push the integration with other existing packages) ;-) A single well-designed package for units treatment would be very useful in this field.

giordano avatar Sep 20 '17 13:09 giordano

@giordano Absolutely. I was actually hoping to integrate this into Cosmology.jl so that we can have comoving_distance, hubble_time, etc all return values with the appropriate units (the _mpc and _gyr suffixes always bothered me). I'll float this idea on the JuliaAstro lists.

mweastwood avatar Sep 20 '17 15:09 mweastwood

Yes, exactly, I also always hated those names, very unJulian!

giordano avatar Sep 20 '17 15:09 giordano

See https://github.com/ajkeller34/Unitful.jl/pull/103 for a prototype implementation of logarithmic units / quantities / etc. Comments welcome.

ajkeller34 avatar Sep 27 '17 05:09 ajkeller34