very minor changes to support Functional units in CDS tables by DShivansh · Pull Request #9971 · astropy/astropy

DShivansh · 2020-02-20T17:44:39Z

Description

changes to support functional units in CDS tables.

pllim

Thanks, @DShivansh ! I have some minor comments below but you can wait for a review from the io.ascii maintainers. I am thinking maybe a change log under 4.1 under "API change" but I can be overruled.

In the meantime, please add test(s).

pllim · 2020-02-20T18:33:49Z

                if unit == '---':
                    col.unit = None  # "---" is the marker for no unit in CDS table
+                elif unit[0] == '[' and unit[-1] == ']':
+                    temp = re.compile(r'\[(?P<un>.*?)\]')


Maybe the compile can be done once at the module level or is that premature optimization?

Yes, I think I should do the compile at the module level because each time it sees the functional unit it will recompile.

pllim · 2020-02-20T18:34:58Z

                unit = match.group('units')
                if unit == '---':
                    col.unit = None  # "---" is the marker for no unit in CDS table
+                elif unit[0] == '[' and unit[-1] == ']':


Is this comparison going to catch other non-CDS stuff and cause unintended side effects?

No because this function is only run when the program guesses that the provided format is CDS.

hamogu

I think this looks good, thanks a lot for working on this. Let's give it a test drive. Can you add a test?
You could just modify https://github.com/astropy/astropy/blob/master/astropy/io/ascii/tests/data/cds.dat so that one column has a logarithmic unit. Alternatively, it might be worthwhile to have a separate test case for that (see the existing cds tests to see how to do that) using a real CDS table with logarithmic units (but shorten the table to only have one line of data to save time on running the test).

mhvk · 2020-02-20T19:16:29Z

Hmm, while this works, I think it is more logically done inside the parser/lexer in units/format/cds.py, so that all the unit machinery benefits. Also, this should round-trip, i.e., I should be able to do u.dex(u.cm/u.s**2).to_string(format='cds') and get out [cm/s2].

DShivansh · 2020-02-21T10:53:39Z

@mhvk parser and lexer have no role when making the table when provided with the data.
I am now working on the to_string functionality.

hamogu · 2020-02-21T10:58:26Z

@DShivansh : @mhvk is right. The Units are parsed here: https://github.com/astropy/astropy/blob/master/astropy/io/ascii/cds.py#L123 That line parses a string to the Unit and then it is the parser/lexer inside the units module that does the work. So, the code that you wrote here should just be moved into the units module, as @mhvk suggests (into the part that deals with "cds" inside the units stuff).

DShivansh · 2020-02-21T11:13:11Z

@hamogu yes, sorry while debugging I stepped over that function 😅, I'll move my code inside
format/cds.py.

DShivansh · 2020-02-21T11:31:33Z

@hamogu I still think that although all the work is done inside the units/format/cds.py but while converting the column info to units to units there is no use of parser

astropy/astropy/units/format/cds.py

Line 81 in 72728ef

def _make_lexer(cls):

and lexer

astropy/astropy/units/format/cds.py

Line 142 in 72728ef

def _make_parser(cls):

Please correct me if I am wrong.

mhvk · 2020-02-21T14:00:18Z

@DShivansh - I will have to correct you, sorry! The line Unit(unit, format='cds', parse_strict='warn') ends up going to format.cds.CDS.parse which uses the CDS parser/lexer.

So, I think you'll need to define [ and ] tokens and associated grammar. Your previous PR on the coordinate directions should give you some idea on how to proceed. The simple test case will be if something like Unit('[cm/s2]', format='cds') gives back u.dex(cm/s2).

p.s. So far, only the format.generic.Generic class has support for logarithmic units, but looking there may still help a little in the implementation.

DShivansh · 2020-02-22T17:48:42Z

@mhvk can you please suggest whether the current implementation is correct or not? Should I now implement the test cases?

mhvk · 2020-02-22T18:31:57Z

@DShivansh - this looks OK, but I must admit that with the parser, I just never know... So, yes, do add test cases, both in units/tests/test_format in test_cds_grammar and (likely a new method) in TestRoundtripCDS, and in io/ascii/tests/test_read.py + test_cds_header_from_readme.py. I fear that adding the test cases may be most of the work for this PR, but it is super-important.

Plus an entry under new features for 4.1. And of course be sure to remove the TODO entry in cds.py (see #9946)!

DShivansh · 2020-02-23T13:07:29Z

@mhvk can you please check whether my to_string implementation is correct or not?

mhvk · 2020-02-23T15:30:51Z

            raise ValueError("Function units cannot be written in {} format. "
                             "Only 'generic', 'unscaled' and 'latex' are "
                             "supported.".format(format))
+        if format == 'cds':


I think this is the right idea, but it should not be here, but rather in DexUnit (which is the only one that is allowed to be overridden; CDS cannot reproduce mag and dB function units). I.e., create a new method to_string there which has your special case for CDS, but otherwise just does return super().to_string(....)

Inside UnitDex what should I do if the format is not cds?

return super().to_string(....)

This comment still stands. We should be sure that, say, u.mag(u.Jy).to_string(format='cds') still raises an error - right now, I think it would incorrectly lead to [Jy]. Indeed, could you add a test that checks that this example continues raises ValueError? (Clearly, I should have done that myself when I introduced the functional units!)

mhvk · 2020-02-23T15:32:23Z

@DShivansh - quick comment in-line. Please do add the test cases even if the code is not working yet, it makes thinking about changes much easier (and will tell you whether you got it right!).

p.s. I'll be traveling the coming week, so responses may be slow.

DShivansh · 2020-02-25T07:31:57Z

@mhvk can you please provide the readme with glob_header and multi_header that have the functional unit.
Can you please tell what all units should I check in TestRoundtripCDS ?
Can you also guide me as to what kind of testing is required in test_read.py?

mhvk · 2020-02-25T08:50:09Z

@DShivansh - sorry, very quickly only: there are examples scattered throughout this PR and the original issue. The latter also has a link to a Vizier table which has them, might as well use the corresponding ReadMe.

pep8speaks · 2020-02-26T08:45:27Z

Hello @DShivansh 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-03-03 20:04:58 UTC

mhvk

@DShivansh - now had a chance to review in a bit more detail. Main comment is about the location of to_string; others are small but might as well address these too.

EDIT: ~~Also needs a changelog entry!~~ Also needs moving the changelog entry to the right section (4.1).

mhvk · 2020-02-29T11:45:13Z

            raise ValueError("Function units cannot be written in {} format. "
                             "Only 'generic', 'unscaled' and 'latex' are "
                             "supported.".format(format))
+        if format == 'cds':


This comment still stands. We should be sure that, say, u.mag(u.Jy).to_string(format='cds') still raises an error - right now, I think it would incorrectly lead to [Jy]. Indeed, could you add a test that checks that this example continues raises ValueError? (Clearly, I should have done that myself when I introduced the functional units!)

mhvk · 2020-02-29T11:48:28Z


        # NOTE THE ORDERING OF THESE RULES IS IMPORTANT!!
        # Regular expression rules for simple tokens
+        def t_OPEN_BRACKET(t):


Looking at format/generic.py for the definition of regular parentheses, I think these two can just be

t_OPEN_BRACKET = r'\[' t_CLOSE_BRACKET = r'\]'

I think we don't need the trailing $ either since the grammar rules only put these at the start and end anyway.

mhvk · 2020-02-29T11:54:31Z

+    data_and_readme = 'data/cdsFunctional.dat'
+    reader = ascii.get_reader(ascii.Cds)
+    table = reader.read(data_and_readme)
+    assert isinstance(table['logg'].unit, type(dex()))


I'd prefer to test explicitly that it is the right unit, so assert table['logg'].unit == u.dex(u.cm/u.s**2).
And might as well test the other units in the table too.

mhvk · 2020-02-29T12:00:51Z

            if len(p) == 3:
                p[0] = Unit(p[1] * p[2])
+            elif len(p) == 4:
+                p[0] = dex(Unit(p[2]))


It doesn't matter too much but I think this can just be p[0] = dex(p[2]) (combined_unit is guaranteed to be a unit instance already).

mhvk

Looks OK, but two remaining little things.

mhvk · 2020-02-29T17:35:41Z

+        if format == 'cds':
+            return "[{}]".format(self.physical_unit.to_string(format=format))
+        else:
+            self_str = self.function_unit.to_string(format)


No need to repeat code from above; as I noted in the original comment, you can just do

return super().to_string(format=format)

mhvk · 2020-02-29T17:36:03Z

    def _quantity_class(self):
        return Dex

+    def to_string(self, format = 'generic'):


No spaces around = for argument defaults.

Works by changing the parser to recognize the square brackets signature used by CDS for Dex. Also adjust Dex.to_string() to be able to output in that format.

mhvk

@DShivansh - looks all OK now, so approving! Note that I saw one left-over import and a missing CR, so I took the liberty to just fetching your PR, fixing those two items, and squashing all commits together (so the edits to io/ascii/cds.py do not enter the history). I also added one further commit with a few round-trip tests (something I had asked for but which slipped through the cracks before) - this ensures the to_string method is properly tested.

DShivansh · 2020-03-04T09:09:02Z

@mhvk thank you for correcting my PR. Can you please tell what is CR? sorry I forgot about the round-trip test case.

mhvk · 2020-03-04T11:38:27Z

@DShivansh - CR=carriage return (was one missing at the end of the data file; really didn't matter much at all). As tests pass, I'll merge this. Thanks again!

astropy-bot Bot added the io.ascii label Feb 20, 2020

pllim requested a review from mhvk February 20, 2020 18:32

pllim added the Enhancement label Feb 20, 2020

pllim added this to the v4.1 milestone Feb 20, 2020

pllim reviewed Feb 20, 2020

View reviewed changes

pllim requested a review from hamogu February 20, 2020 18:37

hamogu reviewed Feb 20, 2020

View reviewed changes

mhvk reviewed Feb 23, 2020

View reviewed changes

DShivansh requested review from hamogu, mhvk and pllim February 26, 2020 08:45

mhvk requested changes Feb 29, 2020

View reviewed changes

mhvk reviewed Feb 29, 2020

View reviewed changes

DShivansh requested a review from mhvk February 29, 2020 17:48

DShivansh and others added 2 commits March 3, 2020 20:37

Support logarithmic units in CDS tables.

77a33d3

Works by changing the parser to recognize the square brackets signature used by CDS for Dex. Also adjust Dex.to_string() to be able to output in that format.

Add round-trip tests for a few dex units

fceb40f

mhvk force-pushed the cds branch from ef6577c to fceb40f Compare March 3, 2020 20:04

mhvk approved these changes Mar 3, 2020

View reviewed changes

mhvk added zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. units labels Mar 3, 2020

mhvk merged commit 6b91476 into astropy:master Mar 4, 2020

astrofrog mentioned this pull request Mar 16, 2021

Ensure that the CDS unit parser can deal with '[-]' #11250

Merged

Suyog7130 mentioned this pull request Jul 27, 2021

Incorrect CDS format unit name string for mag and dB units #11976

Closed

Conversation

DShivansh commented Feb 20, 2020

Description

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamogu left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Feb 20, 2020

Uh oh!

DShivansh commented Feb 21, 2020

Uh oh!

hamogu commented Feb 21, 2020

Uh oh!

DShivansh commented Feb 21, 2020

Uh oh!

DShivansh commented Feb 21, 2020

Uh oh!

mhvk commented Feb 21, 2020

Uh oh!

DShivansh commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Feb 22, 2020

Uh oh!

DShivansh commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Feb 23, 2020

Uh oh!

DShivansh commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Feb 25, 2020

Uh oh!

pep8speaks commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-03-03 20:04:58 UTC

Uh oh!

mhvk 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

DShivansh commented Mar 4, 2020

DShivansh commented Feb 22, 2020 •

edited

Loading

DShivansh commented Feb 23, 2020 •

edited

Loading

DShivansh commented Feb 25, 2020 •

edited

Loading

pep8speaks commented Feb 26, 2020 •

edited

Loading

mhvk left a comment •

edited

Loading