Added a feature which will allow the user to control space b/w value and unit by Telomelonia · Pull Request #14231 · astropy/astropy

Telomelonia · 2022-12-24T18:48:34Z

Description

I added a boolean feature oldstyle in to_string(), giving flexibility to the user to have a space between value and unit.
By default, space should be given in between whenever its possible.
NOTE-For some units like 34'20'' or 11^h22^m no space should be added whether the feature is set to True or False.

oldstyle : bool, optional
            If `False`, include a space between value and unit. If `True`,
            no space is included. Doesn't work with 'latex' or 'unicode'
            format. By default a space is generated.

TODOs

Adding the feature as a boolean keyword
Updating the Docstring
Add a changelog
Update Testing

This pull request is to address ...

Fixes #7455

Testing

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

added explanation for masked array in index.rst for docs

This reverts commit 89de6ad.

This reverts commit c8f5909.

pllim · 2022-12-25T22:23:22Z

The tests need updating. A new test to use the alternate option is also necessary.

Also a change log is necessary (as you have noted in your checklist).

Thanks!

mhvk

Thanks for the PR, but I wonder why this is needed? Most journals insist that a space is used between a number and the unit. Also, while for simple units this might make sense, it does not generalize to composite units (which have spaces in between them).

Overall, my tendency would be to leave this for people to sort for themselves if they want it; generally .to_string(...).replace(' ', '') will be all that's needed.

So, not in favour; marking as "request changes" since if we were to go for adding this option, it definitely should have a different name.

mhvk · 2022-12-28T01:00:36Z

Oops, I'm so sorry, I hadn't even realized that the issue here is to fix that angles do not have spaces between the number and the unit. Very silly of me; I should have read the issue.

Now having done so, I agree with the comments there that the default should be to have the space, with the only exception being when one of the special symbols is used. My tendency would be not to even have an option for this.

Anyway, now in favour as long as we change the default!

mhvk

A few more thoughts.

mhvk · 2022-12-28T01:03:34Z

                    unit_string = unit_string[1:-1]
                format_func = func
-                func = lambda x: format_func(x) + unit_string
+                # Add a space if no format(i.e unicode or latex) is given


I wonder if the whole else clause could simply leave it to Quantity.to_string() via

return super().to_string(unit=unit, precision=precision, format=format)

Hmm nice, that will simplify the code ... let me review it again

Added a space between value and unit and added a feature of old style if someone wants to use the old style no space string

Telomelonia · 2023-02-08T12:44:00Z

@pllim
I have a problem regarding the pre-commit.ci check. Apart from that all is good to go

pllim · 2023-02-08T13:21:21Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

pllim

A test need to be added with oldstyle=True to ensure it works as intended.

Please rename the change log fragment file from 14231.feature.rst to 14231.api.rst as suggested by reviewer. Change log text also need to elaborate what this oldstyle actually does and why it is needed now.

Needs to squash all commits into one before this is ready for merge too. To do that, please remember to pull in pre-commit autofix commit from GitHub back to your local checkout first. Feel free to ask if you are unsure how to do this part.

mhvk · 2023-02-08T14:06:05Z

I think there is still one open question: as I commented above, I wonder if we even need the oldstyle option -- I think what we had was just wrong. What do others think?

p.s. If we do go with an option, we should give it a better name (and default should definitely be off).

eerovaher · 2023-02-08T16:53:50Z

I have a problem regarding the pre-commit.ci check. Apart from that all is good to go

pre-commit can fix most (if not all) issues it highlights, see https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit

Co-authored-by: P. L. Lim <[email protected]>

Telomelonia · 2023-02-09T12:17:04Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

pllim · 2023-02-09T13:45:06Z

Re: #14231 (comment)

@mhvk , I think that is a discussion for astropy-dev mailing list and/or dev telecon. I am going to turn this PR into draft until this discussion is resolved.

mhvk · 2023-02-09T18:50:48Z

Let's just ping the coordinate team - @eerovaher, @StuartLittlefair, @adrn, @eteq - on whether to make a space between a number and the unit the standard case (as it is for Quantity), or whether it is important to allow users to keep the "old style" where, for Angle only, the space was omitted (likely by mistake). I feel this is a quite clear case of only one correct way, and that effectively this is a bug fix, with little or no impact to anyone since it affects only how angles are shown in string form. So, I'm in favour of just changing, without an option to get the old behavor back (which, after all is just a f"{angle.value}{angle.unit}" away, probably easier to discover than a keyword argument!).

eerovaher · 2023-02-09T20:24:03Z

I agree with @mhvk that there is no need to make the "old style" option available as a keyword argument to Angle.to_string(). In some cases with Unicode, one of the LaTeX formats or sexagesimal representation the space should be omitted (e.g. 1°00′00″ is correct as is), but that should be automatically taken care of by to_string() and should not rely on manual intervention by the users. The space can still be omitted by handling the value and unit separately, like @mhvk showed, and this technique is already documented:

astropy/docs/units/format.rst

Lines 45 to 52 in 24a4ee1

    
           To format both the value and the unit separately, you can access the |Quantity| 
        
           attributes within format strings:: 
        
               >>> q = 10.5 * u.km 
        
               >>> q 
        
               <Quantity  10.5 km> 
        
               >>> f"{q.value:.3f} in {q.unit}" 
        
               '10.500 in km'

StuartLittlefair · 2023-02-10T12:48:10Z

Just to sat that in response to @mhvk's ping, I have no strong opinion on this. The only real issue I can anticipate is if third-parties had based their unit tests on the string output from angles, in which case I don't have much sympathy...

Telomelonia · 2023-02-13T19:36:29Z

I thinking of making another PR to address this issue as just a bug where a space is required in the result in between value and unit.

mhvk · 2023-02-13T20:19:46Z

@Telomelonia - it might indeed be good to have 2 PRs to compare, though it also is fine to work with this one, Thanks for persisting!!!

Telomelonia added 5 commits December 23, 2022 20:59

added explanation for masked array

89de6ad

added explanation for masked array in index.rst for docs

Revert "added explanation for masked array"

6957eb0

This reverts commit 89de6ad.

Update angles.py

c8f5909

Revert "Update angles.py"

e08b2b3

This reverts commit c8f5909.

Merge branch 'main' of https://github.com/Telomelonia/astropy

60d82d5

github-actions Bot added the coordinates label Dec 24, 2022

pllim added this to the v5.3 milestone Dec 25, 2022

pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Dec 25, 2022

pllim requested review from Cadair, astrofrog and larrybradley December 25, 2022 22:23

mhvk requested changes Dec 28, 2022

View reviewed changes

Comment thread astropy/coordinates/angles.py Resolved

mhvk reviewed Dec 28, 2022

View reviewed changes

Merge branch 'astropy:main' into main

09c7ede

larrybradley removed their request for review February 3, 2023 19:00

Telomelonia changed the title ~~Added a feature which will allow the user to control space b/w value and unit [ci skip]~~ Added a feature which will allow the user to control space b/w value and unit Feb 8, 2023

Telomelonia force-pushed the value_space_unit branch 2 times, most recently from 947ad3a to 5e9730a Compare February 8, 2023 11:10

Added space as default between unit and value

e559b02

Added a space between value and unit and added a feature of old style if someone wants to use the old style no space string

Telomelonia force-pushed the value_space_unit branch from 5e9730a to e559b02 Compare February 8, 2023 11:11

[pre-commit.ci] auto fixes from pre-commit.com hooks

03622c8

for more information, see https://pre-commit.ci

pllim reviewed Feb 8, 2023

View reviewed changes

Comment thread astropy/coordinates/angles.py Resolved

Comment thread astropy/coordinates/angles.py Resolved

Telomelonia and others added 2 commits February 9, 2023 16:59

Update astropy/coordinates/angles.py

c173010

Co-authored-by: P. L. Lim <[email protected]>

Update astropy/coordinates/angles.py

de8d265

Co-authored-by: P. L. Lim <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

0ac1c6c

for more information, see https://pre-commit.ci

pllim marked this pull request as draft February 9, 2023 13:45

pllim added the needs-discussion label Feb 9, 2023

Telomelonia mentioned this pull request Feb 13, 2023

Bug fix of no space in the result of to_string. #14379

Merged

3 tasks done

pllim closed this in #14379 Apr 19, 2023

Telomelonia deleted the value_space_unit branch April 25, 2023 05:01

Telomelonia restored the value_space_unit branch April 25, 2023 05:01

Conversation

Telomelonia commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODOs

Testing

Checklist for package maintainer(s)

Uh oh!

pllim commented Dec 25, 2022

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Dec 28, 2022

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

Telomelonia Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Telomelonia commented Feb 8, 2023

Uh oh!

pllim commented Feb 8, 2023

Uh oh!

pllim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhvk commented Feb 8, 2023

Uh oh!

eerovaher commented Feb 8, 2023

Uh oh!

Telomelonia commented Feb 9, 2023

Uh oh!

pllim commented Feb 9, 2023

Uh oh!

mhvk commented Feb 9, 2023

Uh oh!

eerovaher commented Feb 9, 2023

Uh oh!

StuartLittlefair commented Feb 10, 2023

Uh oh!

Telomelonia commented Feb 13, 2023

Uh oh!

mhvk commented Feb 13, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Telomelonia commented Dec 24, 2022 •

edited

Loading

pllim left a comment •

edited

Loading