add basic CDS writer, without ReadMe - #11835

Hello @Suyog7130 👋! 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 2021-06-27 17:45:35 UTC

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

Thanks for opening the PR! Glad to see code already 😄

Before I take a close look and give you detailed comments, I want to note a few things:

1. To skip running continuous integration tests until your PR is ready, you should add [ci skip] or [skip ci] to your commit messages. You have added [skip ci] to the PR message which won't work.
   To give you an example, added CDS ReadMe template files and CDScolumn.py was a commit message you wrote. This should be changed to added CDS ReadMe template files and CDScolumn.py [ci skip].
2. There are a few PEP8 issues that you should fix in order to follow coding convections - look at the comment above from pep8speaks. If you want your code editor to automatically detect PEP8 errors, you can use Emacs with the following setup: https://docs.astropy.org/en/latest/development/codeguide_emacs.html. I use it to check if my code is following conventions so that I can fix issues before I push them.
3. I noticed that you commented out code that you wanted to remove. Any code that is not needed should be removed, not commented out. Comments are only meant for providing description of code/implementation.
4. 1,172 lines of code is a lot to go through at one time! I would suggest that instead of adding the entire file from cdspyreadme, you work in a modular way, i.e. you add one component of the reader with tests to show it works, and work on top of it. Your goal is to understand cdspyreadme, and implement it within Astropy in an efficient way, and our goal is to make sure your code works properly. It will be easier for us and you to keep track of changes if each change is small and separately added (maybe as a commit or as a separate PR).

This might seem like a lot of comments, but they are meant to help you throughout your GSoC work (please don't be disheartened!) This is just the first week so we don't expect you to have everything figured out (that is why GSoC is a few months long 😃). We can discuss this more next week!

Hi, I have got the Cds.write function working now. See the latest commit.
Kindly don't mind the code styling related stuff as of now. I can correct those in sometime.
Also, it looks like the commit message at the time of first starting the Pull Request needs to have [skip ci] in it. The same on later commits doesn't seem to work, since all the tests are getting rerun.

Don't worry if the tests run but should not. I've seen the [skip ci] fail in other cases, too. Not sure why. We try to save the electricity, but if it does not work, then it does not work and it's not our job to debug that.

@Suyog7130 and mentors. Just saw this and didn't realize this was a GSoC project, but great!

One question I have is about the flavor of CDS and whether to focus on the AAS Machine Readable Table (MRT, https://journals.aas.org/mrt-overview/) variant. In the original thread it was suggested to write as AAS MRT instead of CDS (#11257 (comment)). Certainly for publishing to AAS journals this is what is needed.

Who knows, maybe you can do both (as implied in the toMRT method), but they are enough different in the metadata and format that MRT will definitely need its own format mrt and reader / writer, even if some of the code can be shared. I was also imagining that the meta data will be nicely structured, meaning that the author list would really be a Python list of authors, and column notes would be correctly parsed and associated with a column, etc.

A few thoughts:

• This is going to need a lot of tests since there is a lot going on. 😄
• I suspect that a lot of the sexigesmal handling should be replace by astropy angle parsing functionality, though I haven't looked in detail.
• If I were doing this I probably would have looked at cds.pyreadme for inspiration and some functions, but written most of it from scratch. This code seems a bit more complicated than it has to be to go from astropy Table to MRT, in part because it supports functionality that is not needed like handling multiple tables. I am not sure the writer needs to use separate classes like CDSAstropyTable (see the classic talk https://www.youtube.com/watch?v=o9pEzgHorH0).
• If you do end up sticking with this code, you'll need to add a lot of comments explaining how/why the code works. At present there are basically none.

I think CDS and AAS formats are close enough that it's easy to do the other one, once we have one of them in place or to declare features where they differ to be beyond the scope of this reader. I don't have strong feelings about which one to start with. I fact, I thought that there are compatible (we read them with the same reader), with maybe some difference on the default naming convention for columns.

Notably, some of the associations between columns that the standard talks about (https://journals.aas.org/mrt-labels/#reflab) will always be left to the user - astropy has no way of knowing that column B holds "a note or remark" on the entries of column A and thus column be should be named "n_A". In that sense, the reader will be functionally OK, but it will probably require the user to rename the columns appropriately before writing in Python or after writing with a simple text editor on the written out column. I think that's not a problem - journal have data editors who will check this stuff.

Other notes:

• We talked about the relation to pyCDS, how to use (or not) their code and how significant parts of their code are not needed because we can e.g. use the fixed-column-width writer already.
• We have the same suspicion on the sexagesimal stuff and will concentrate on other columns first. Of course, coordinates are important, but the problem is almost entirely disjoint from the rest of the format.
• We are looking at the LaTeX reader/writer for handling extra information that "normal" tables don't have (authors, bibcode, ...). Some of that might be taken from/written to table.meta entries, other things might come in separately as in latexdict.

We welcome your thoughts on how to do that best @taldcroft. One of the downsides of "GSOC + github" is that github does not cover the entire discussion because some of it happens live on zoom.

I think CDS and AAS formats are close enough that it's easy to do the other one, once we have one of them in place or to declare features where they differ to be beyond the scope of this reader.

The Byte-by-byte Description of file section (including the label and unit rules) and the fixed-format data columns are essentially the same. However, all the other metadata are very different. Compare the MRT header (https://journals.aas.org/mrt-standards/) to the CDS header (http://vizier.u-strasbg.fr/doc/catstd-3.1.htx). As I said there is probably a lot of the code that can be shared, but they need to be considered distinct formats (mrt vs cds) in astropy. If you try to read a CDS file with mrt it should fail because the header meta data is all wrong.

Agreed that the label standards are in ways difficult to enforce, but there are opportunities to use the standard for better integration with astropy mixins. In particular a SkyCoord column can be represented in MRT/CDS, and likewise a Time column has a recommended MRT/CDS representation.

I understand this is still early in the code development, but at some point the variable and method naming will need to comply with PEP8 in order to be merged. Module level constants should be SCREAMING_SNAKE_CASE, and method and variable names should be lower case like init_cds_columns. I'll try to mostly keep my nose out of the details, but I do have a strong interest in maintaining the coding style of the io.ascii package.

@Suyog7130 - can you please try again with the [skip ci] in every commit message while you are still pushing commits frequently without a need for full CI testing? We really don't like wasting CI time. If there is a problem in our Actions not skipping then a maintainer will report this to the right people.

Hi @taldcroft and others,

Thanks for reaching out and for the valuable suggestions.
Special thanks for that PyCon 2012 video. It was insightful. :)

I had meant this PR as a pseudo-work space where the changes I make can be viewed and discussed by others. This PR is not meant to be the final one(s) which will be put up for merging. I shall squash and remove edits that are not required before doing so. That's why this PR is marked Draft.

I wanted to have something working quite early in the project, in order to be able to pinpoint accurately when something doesn't work. This is why I started with directly adding the cdspyreadme code within Astropy. Afterwards, I am also writing the code from scratch. As more of the required features from cdspyreadme get integrated into cds.py, those files and codes added earlier will be removed.

About the reading/writing to MRT, in fact I wrote about it briefly in my GSoC Proposal that I could attempt it as an extension. I don't have an opinion on whether or not it should have it's own format classes etc. However, since the title of my GSoC project is to Add a CDS format writer to Astropy, I would prefer to work on the CDS format writer first and then on the MRT format. The MRT header anyway appears to be a bit simpler than the CDS header, so there shouldn't be much difficulty in the extension.

So, in a nutshell, this is my workflow:

• Try out directly using cdspyreadme from within Astropy.
• Add CdsData.write method.
• Add a ByteByByte writer.
• Write features to add complete ReadMe to the Header, starting off with having both ReadMe and Data in a single file.
• Have features for writing separate CDS ReadMe and Data file.
• Further work on some specific table columns, for instance, those containing Units and Coordinates.
• Add appropriate tests along the way.
• Resolve other issues that come up.
• MRT format reader/writer.

I have completed the first three tasks and will now work on the fourth. I think by the time this finishes, a separate CDSColumn.py won't be required.
As discussed sometime back, I can open another PR which adds the Data writer, in the meantime.

The Byte-by-byte Description of file section (including the label and unit rules) and the fixed-format data columns are essentially the same. However, all the other metadata are very different. Compare the MRT header (https://journals.aas.org/mrt-standards/) to the CDS header (http://vizier.u-strasbg.fr/doc/catstd-3.1.htx). As I said there is probably a lot of the code that can be shared, but they need to be considered distinct formats (mrt vs cds) in astropy. If you try to read a CDS file with mrt it should fail because the header meta data is all wrong.

There's a tool anafile (http://vizier.u-strasbg.fr/doc/anafile.htx) available from CDS which checks if a given ReadMe matches the table data or not. I think I can later on write something similar to match the table data with the ReadMe. This would raise an error when MRT file is been read using CDS reader and vice-versa.

I understand this is still early in the code development, but at some point the variable and method naming will need to comply with PEP8 in order to be merged. Module level constants should be SCREAMING_SNAKE_CASE, and method and variable names should be lower case like init_cds_columns. I'll try to mostly keep my nose out of the details, but I do have a strong interest in maintaining the coding style of the io.ascii package.

Yup! I would keep it in mind. If I overlook something, it can of course be corrected towards the end before the PR is merged. Following Aarya's first comment about the PEP8 I have started to use tox -e codestyle to check styling errors as well.

@Suyog7130 - can you please try again with the [skip ci] in every commit message while you are still pushing commits frequently without a need for full CI testing? We really don't like wasting CI time. If there is a problem in our Actions not skipping then a maintainer will report this to the right people.

Looks like all but two tests are getting skipped now, which is good. I had tried adding [skip ci] to every commit before this as well, but around 6 tests were still been run.

@Suyog7130 - thanks for the great response and summary. Just some quick points on priority:

Have features for writing separate CDS ReadMe and Data file.

I'm not aware of any interest in the community for being able to write separate files like this. Maybe I'm just missing this, but every time this has come up, people are strongly opposed to the separate header / data file. In general this is just causes problems.

On the other hand, during discussion of writing CDS-style files, there was unanimous agreement that the MRT format should be the priority, including from the author of cds.pyreadme. So I just ask that you keep that in mind and make sure there is time for MRT.

Discussions continued in #11897.