Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feedback? #1

Open
mweastwood opened this issue Jun 20, 2017 · 7 comments
Open

feedback? #1

mweastwood opened this issue Jun 20, 2017 · 7 comments

Comments

@mweastwood
Copy link
Member

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
  • Decibel-based unit support PainterQubits/Unitful.jl#41 would be really cool to have because astronomy has a bunch of extremely confusing logarithmic units

Any comments?

@mweastwood
Copy link
Member Author

mweastwood commented Jun 20, 2017

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

@ajkeller34
Copy link

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.

@mweastwood
Copy link
Member Author

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).

@giordano
Copy link
Member

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.

@mweastwood
Copy link
Member Author

@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.

@giordano
Copy link
Member

giordano commented Sep 20, 2017

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

@ajkeller34
Copy link

See PainterQubits/Unitful.jl#103 for a prototype implementation of logarithmic units / quantities / etc. Comments welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants