Skip to content

Add "latex_inline" unit format representation with negative powers instead of fractions - #2622

Merged
mdboom merged 3 commits into
astropy:masterfrom
zblz:inline-latex-units
Aug 26, 2014
Merged

Add "latex_inline" unit format representation with negative powers instead of fractions#2622
mdboom merged 3 commits into
astropy:masterfrom
zblz:inline-latex-units

Conversation

@zblz

@zblz zblz commented Jun 13, 2014

Copy link
Copy Markdown
Member

This PR adds a new unit format representation called latex_inline which uses negative powers instead of fractions for expressions of rate. It is otherwise equivalent to the latex format. This format is the one used in the IAU Style Manual and required by some publications (e.g., ApJ and AJ).

>>> fluxunit = u.erg / (u.cm ** 2 * u.s)
>>> fluxunit.to_string('latex')
u'$\\mathrm{\\frac{erg}{s\\,cm^{2}}}$'
>>> fluxunit.to_string('latex_inline')
u'$\\mathrm{erg\\,s^{-1}\\,cm^{-2}}$'

@zblz zblz changed the title Add latex_inline unit format representation with negative powers instead of fractions Add "latex_inline" unit format representation with negative powers instead of fractions Jun 13, 2014
@zblz

zblz commented Jun 13, 2014

Copy link
Copy Markdown
Member Author

I tried to reduce code duplication by changing the behaviour of the Latex class based on whether _inline is found in the class name self.__class__.__name__, and then adding a class Latex_inline(Latex). Is this considered good coding style or is there a better way to do it without duplicating the to_string method?

@mdboom

mdboom commented Jun 13, 2014

Copy link
Copy Markdown
Contributor

👍 for the functionality. I'm not sure that testing on the specific name of the class is a great idea, though. It will break if the class is ever further subclassed, for example. I would instead add a new private method, e.g. _format_bases, that in Latex does what we have now, and in LatexInline does the negative power version. Then update tostring to call self._format_bases where appropriate.

@mdboom mdboom added the units label Jun 13, 2014
@zblz

zblz commented Jun 16, 2014

Copy link
Copy Markdown
Member Author

I had not thought about further subclassing, your suggestion is definitely better. I updated the PR with a _format_bases method.

@mdboom mdboom added this to the v0.4.1 milestone Jun 16, 2014
@mdboom

mdboom commented Jun 16, 2014

Copy link
Copy Markdown
Contributor

Great. This is 👍 from me. The only thing that's missing is an entry in CHANGES.rst, but I think this will end up in 0.4.1 (which we don't have a section for yet). We can clean that up/finalize it after the impending 0.4.0 release.

@astrofrog

astrofrog commented Jul 16, 2014

Copy link
Copy Markdown
Member

@mdboom - should this be included in 1.0 since it's technically a new feature?

@mdboom

mdboom commented Jul 16, 2014

Copy link
Copy Markdown
Contributor

Yeah -- you're probably right. Remilestoning to 1.0

@mdboom mdboom modified the milestones: v1.0.0, v0.4.1 Jul 16, 2014
@astrofrog

astrofrog commented Jul 16, 2014

Copy link
Copy Markdown
Member

@zblz - could you rebase this to include the latest changelog changes, and add an entry to the 1.0 section?

@zblz

zblz commented Jul 17, 2014

Copy link
Copy Markdown
Member Author

@astrofrog @mdboom: Done

mdboom
mdboom reviewed Jul 17, 2014
View reviewed changes
Comment thread astropy_helpers Outdated

@mdboom mdboom Jul 17, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow, the hash to astropy_helpers is changed by this PR. Since that's unrelated to the problem at hand, we probably shouldn't do that here.

@astrofrog astrofrog Jul 17, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this happen several times. I wonder what workflow leads to this happening and what would avoid it? We should add instructions to the CONTRIBUTING.md file.

@embray embray Jul 17, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way this would happen if someone wrote git add astropy_helpers, which they shouldn't unless they intended to.

@astrofrog astrofrog Jul 17, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, then it could also be git commit -a, or maybe the GitHub Mac client which I've seen do the equivalent of git commit -a. I guess what I'm curious about is why the hash is changing in the first place?

@zblz zblz Jul 24, 2014

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed used git commit -a, so that might be the reason. I will roll back this change. This should probably be added to CONTRIBUTING.md.

@embray embray Jul 24, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zblz sorry for the confusion. I'm looking into ways to make this less problematic when committing changes.

@embray embray mentioned this pull request Aug 13, 2014
Merged
@embray

embray commented Aug 13, 2014

Copy link
Copy Markdown
Member

Note, due to the change in 8944678 this PR now depends on #2852 . @mdboom: any thoughts on this? Seems straightforward enough to me.

@zblz

zblz commented Aug 13, 2014

Copy link
Copy Markdown
Member Author

Rebased to current master and adapted to use @embray's automatic formatting register of #2852. Note: do not merge before #2852 is merged and this PR rebased on top of that.

@mdboom

mdboom commented Aug 25, 2014

Copy link
Copy Markdown
Contributor

This looks good. I've merged #2852 and restarted Travis here. I'm find with merging once all Travis failures are resolved.

@zblz
zblz force-pushed the inline-latex-units branch from 8944678 to 20f9c48 Compare August 26, 2014 07:31
@zblz

zblz commented Aug 26, 2014

Copy link
Copy Markdown
Member Author

Rebased to use new formatting register, now everything should work fine.

mdboom added a commit that referenced this pull request Aug 26, 2014
@mdboom
Merge pull request #2622 from zblz/inline-latex-units
96934cd 
Add "latex_inline" unit format representation with negative powers instead of fractions
@mdboom
mdboom merged commit 96934cd into astropy:master Aug 26, 2014
embray
embray reviewed Aug 26, 2014
View reviewed changes
Comment thread CHANGES.rst

@embray embray Aug 26, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is under "Bug Fixes" but it should probably be under "New Features"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

units

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@zblz @mdboom @astrofrog @embray