Add QDP data file handling by matteobachetti · Pull Request #11256 · astropy/astropy

matteobachetti · 2021-01-18T13:59:11Z

Description

This pull request is to address Issue #8382, adding QDP data handling.

pep8speaks · 2021-01-18T13:59:17Z

Hello @matteobachetti 👋! 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-04-08 11:55:44 UTC

embray

This is helpful for understanding how this file format is interpreted and the code is very well formatted. I think it can be simplified though.

embray · 2021-01-18T21:04:59Z

+    if input_colnames is None or len(input_colnames) == ncol:
+        colnames = interpret_err_lines(
+            err_specs, ncol, input_colnames=input_colnames
+        )


I don't really understand what this is doing. Is it initializing the err_specs data structure?

It is reading err_specs and understanding if some columns represent the uncertainty of others. Main column are named col1, col2, ... or whatever the user gives (e.g. MJD, Rate). Error column are named col0_err (or MJD_err) for symmetric error, col0_perr and col0_nerr for positive/negative error

I do this once at the start of the file, assuming that err_specs is empty, for the case where the file does not contain the READ [ST]ERR commands. This gets overwritten when these commands are found.

matteobachetti · 2021-01-19T11:46:07Z

I don't understand what is happening in those failed tests. It looks like the logger got confused, but only in one test case :O

pllim

Probably need user facing doc too.

cc @PaulKuin

matteobachetti · 2021-01-20T10:00:54Z

@embray: I think I fixed most fixable things :). I left a couple more requests for clarifications or minor "rebuttals"
@pllim: I added something to the main Read/Write docs, feel free to point out more documents to update

taldcroft

@matteobachetti - thanks, this will be a nice addition!

One thing that is going to complicate matters is that this should really be made part of io.ascii. The QDP format is very much an ASCII table format so it should live in that subpackage and support the user doing ascii.read('data.qdp', format='qdp', names=['x', 'y', 'z']). The machinery in io.ascii is a bit orthogonal to the way you've written this so far. If you just take the first step of moving it into io.ascii I can help with getting it to fit in.

matteobachetti · 2021-01-21T17:17:32Z

Hi @taldcroft, I started moving things around. Alas, while the io.misc machinery is more or less straightforward, I still haven't understood how io.ascii works. I need some help to plug my functions in the correct places :)

taldcroft · 2021-01-21T18:02:36Z

I need some help to plug my functions in the correct places.

Yes. The io.ascii machinery is very general and extensible but not very easy to understand. (I'd make it simpler if I wrote it today...). The easiest is if I just play around with it once you have done the basic changes, since I cannot from the top of my head tell you the path.

It looks like you moved the code around. Are there other changes you are planning?

matteobachetti · 2021-01-21T18:56:41Z

It looks like you moved the code around. Are there other changes you are planning?

Not until I understand a little more :). I drafted a couple of classes based on the functionality in basic.py, but I need to understand some very basic stuff (For example: what delimiter should I give in QDPsplitter for "any number of spaces"?)

taldcroft · 2021-01-21T20:36:13Z

For example: what delimiter should I give in QDPsplitter for "any number of spaces"?

The delimiter ' ' implies any number of spaces. This is delegated to the standard library csv.reader.

matteobachetti · 2021-03-17T08:22:42Z

@taldcroft I don't understand how to make read accept the table_id keyword

taldcroft · 2021-03-17T10:56:09Z

@matteobachetti - I pushed a commit that seems to do the trick.

One comment, can you use names instead of input_colnames for API consistency with io.ascii? An analog is the no_header format where supplying names will define the column names.

matteobachetti · 2021-03-17T11:01:25Z

Thanks! I'll change the keyword to names as requested (and I guess there will be some more API fixes like this to do)

taldcroft · 2021-03-17T11:03:34Z

There are a few flake8 issues that should be cleaned up:

(astropy) ➜  astropy git:(add_qdp) flake8 astropy/io/ascii/qdp.py --ignore=E402,W503,F541 --max-line-length=100 
astropy/io/ascii/qdp.py:9:1: F401 'itertools.groupby' imported but unused
astropy/io/ascii/qdp.py:14:1: F401 'astropy.io.registry as io_registry' imported but unused
astropy/io/ascii/qdp.py:29:101: E501 line too long (131 > 100 characters)
astropy/io/ascii/qdp.py:443:5: F401 'astropy.io.ascii' imported but unused

taldcroft · 2021-03-17T11:36:54Z

+        self.input_colnames = input_colnames
+
+    def read(self, qdp_file):
+        return read_table_qdp(qdp_file, table_id=self.table_id,


Another key point for API consistency is to allow the input to be a filename, a string with the data, a list of strings or a file object. This is normally done using the following:

# Get a list of the lines (rows) in the table self.lines = self.inputter.get_lines(table, newline=newline)

Then the core table parsing should always expect a list of lines. I see you have support for providing the table data as a string, though this is not obvious from the docstrings.

To go 100% in the style of io.ascii, the standalone routines like read_table_qdp would not be considered part of the public API and then you would rewrite that function and all the downstream ones to accept only a list of lines.

Another option is to adapt get_lines_from_file to support the standard input options.

matteobachetti · 2021-03-17T11:46:57Z

@taldcroft next: I'm trying to supersede the write interface, but I don't understand how to do that. I commented out my definition of write so that tests pass, but at the moment the interface isn't able to write proper QDP files (with SERR/TERR instructions)

matteobachetti · 2021-03-17T11:49:41Z

@taldcroft, I just noticed that you reviewed the code while I was making the last modifications. I'll work on your comments. Thanks!

taldcroft · 2021-03-17T17:56:31Z

More changes (I made these locally for testing):

diff --git a/astropy/io/ascii/__init__.py b/astropy/io/ascii/__init__.py
index e10998ac1..9e1b79a19 100644
--- a/astropy/io/ascii/__init__.py
+++ b/astropy/io/ascii/__init__.py
@@ -35,6 +35,7 @@ from .latex import Latex, AASTex, latexdicts
 from .html import HTML
 from .ipac import Ipac
 from .daophot import Daophot
+from .qdp import QDP
 from .sextractor import SExtractor
 from .fixedwidth import (FixedWidth, FixedWidthNoHeader,
                          FixedWidthTwoLine, FixedWidthSplitter,
diff --git a/astropy/io/ascii/qdp.py b/astropy/io/ascii/qdp.py
index d6af72700..a62d2d6c0 100644
--- a/astropy/io/ascii/qdp.py
+++ b/astropy/io/ascii/qdp.py
@@ -536,7 +536,6 @@ class QDP(basic.Basic):
 
     """
     _format_name = 'qdp'
-    _io_registry_format_aliases = ['qdp']
     _io_registry_can_write = True
     _io_registry_suffix = '.qdp'
     _description = 'Quick and Dandy Plotter'

The io_registry_format_aliases should not be used in this case. It does require that for reading with Table, the format is given as ascii.qdp instead of just qdp but this is the preferred syntax for clarity.

matteobachetti · 2021-03-18T11:36:36Z

@taldcroft thanks a lot for your thorough review until now, I think I addressed all of your comments :)

matteobachetti · 2021-03-18T11:38:17Z

There is a failure I don't understand. Aren't warnings allowed?

taldcroft · 2021-03-19T10:42:31Z

@matteobachetti - uncaught warnings are not allowed in testing, they are converted to errors as you've seen. Using https://docs.pytest.org/en/stable/warnings.html#recwarn you need to catch them and test that the expected warning got emitted.

matteobachetti · 2021-03-19T14:15:27Z

@taldcroft But I do test it, in test_qdp.py. This error comes from the generic test in test_write.py. I guess this assumes that ascii readers don't emit warnings with default values, but in this case, this reader is made for files that can contain multiple tables, and it has to warn the user if the user does not specify which table to read.

taldcroft · 2021-03-19T15:45:55Z

@matteobachetti - you can put in some special-case handling for the qdp format to update kwargs so the test passes (if indeed it fully supports writing masked data). I.e. add the appropriate table_id (if I have gotten things right).

matteobachetti · 2021-03-24T08:05:34Z

@taldcroft I don't think the failure has to do with my PR, so whenever you want, this is ready for review.

taldcroft

@matteobachetti - I took another pass and there are just docs and minor test issues to be fixed. One key thing is that you need to rebase on current master and update this to reflect the new change log system. See emails to astropy-dev (ANN: astropy plans to switch to towncrier for changelog management), in particular the one from Simon that talks about how to fix existing PRs. Hopefully the rebase will fix the CI failure as well.

Welcome to astropy development, it's never quite easy! 😄

taldcroft · 2021-04-07T16:07:08Z

+    assert np.ma.is_masked(t['b'][0])
+    assert np.isnan(t['d'][1])
+
+    t.values_equal(dat)


You need to check that all the values are actually equal:

for col_equal in t.values_equal(dat).itercols(): assert np.all(col_equal)

But it turns out that doesn't work because of the nan, so you need instead:

for col1, col2 in zip(t.itercols(), dat.itercols()): assert np.isclose(col1, col2, nan_equal=True)

taldcroft · 2021-04-07T16:17:37Z

+    t.write(test_file, err_specs={'terr': [1], 'serr': [3]})
+    t2 = Table.read(test_file, names=['a', 'b', 'c', 'd'], table_id=0)
+
+    t.values_equal(t2)


Same as above.

taldcroft · 2021-04-07T16:17:57Z

+    t.write(test_file, err_specs={'terr': [1], 'serr': [3]})
+    t2 = Table.read(test_file, names=['a', 'b', 'c', 'd'], table_id=0)
+
+    t.values_equal(t2)


taldcroft · 2021-04-07T16:19:55Z

+    return lines
+
+
+# def _analyze_qdp_file(qdp_file):


Remove this commented-out function if it's not needed.

taldcroft · 2021-04-07T16:25:02Z

+
+    Parameters
+    ----------
+    err_specs : dict, {'serr': [n0, n1, ...], 'terr': [n2, n3, ...]}


The top line should just be err_specs : dict (i.e. only specify the type) with the details in the next line.

See https://github.com/astropy/astropy/pull/11118/files for examples of where this is coming from.

taldcroft · 2021-04-07T16:27:12Z

+
+    Other parameters
+    ----------------
+    names : list of strings


list of str

taldcroft · 2021-04-07T16:28:29Z

+
+    Returns
+    -------
+    tables : list of `Table` objects


list of `~astropy.table.Table`

taldcroft · 2021-04-07T16:29:44Z

+        Number of the table to be read from the QDP file. This is useful
+        when multiple tables present in the file. By default, the first is read.
+
+    names : list of strings


Docstring parameters should have the same order as in the function. Also, list of str

taldcroft · 2021-04-07T16:30:08Z

+
+    Returns
+    -------
+    tables : list of `Table` objects


list of `~astropy.table.Table`

taldcroft · 2021-04-07T16:42:02Z

+
+
+class QDP(basic.Basic):
+    """QDP table.


This main docstring should be expanded to illustrate the available parameters and usage. This will be the only way people know about the header_id, err_specs etc. For a good example see the following that shows a full example of the file format and how to read. You can also include a write example.

astropy/astropy/io/ascii/daophot.py

Line 315 in 1ed1d94

class Daophot(core.BaseReader):

matteobachetti · 2021-04-07T19:54:12Z

@taldcroft should be done, including the rebase. I'm not sure the CHANGES.rst file is correct after rebase though.

pllim · 2021-04-07T20:14:41Z

We don't use CHANGES.rst anymore, see https://github.com/astropy/astropy/blob/main/docs/changes/README.rst

pllim · 2021-04-07T20:15:48Z

@saimn , is squashing necessary here to change the changelog ?

matteobachetti · 2021-04-07T20:37:02Z

The failures should be unrelated. The failing test is about a config file, and unit tests were passing up until 04db56e (the only failure was from readthedocs, bad formatting of a docstring)

pllim · 2021-04-07T20:43:47Z

That is weird (the config failure). Luckily, that test is getting removed in #10877 . Unluckily, it failing is blocking the rest of the jobs for you.

saimn · 2021-04-07T21:01:42Z

@saimn , is squashing necessary here to change the changelog ?

No, just revert the changes in CHANGES.rst (git checkout master -- CHANGES.rst)

pllim · 2021-04-07T21:34:58Z

Update: CI is fixed now. A rebase would help. FYI

matteobachetti · 2021-04-08T08:03:19Z

@pllim @taldcroft I renamed my fork's master branch to main, rebased this branch, and eliminated the changes to CHANGES.rst as suggested. Thanks for your help :)

matteobachetti · 2021-04-08T08:07:10Z

Due to the trial-and-error path I followed and the initial proposed addition to io.misc then moved to io.ascii, there are many commits in this PR. I might give a stab at reducing the number of commits by rebasing interactively one last time, if you think it's useful (or you might just squash and merge when satisfied with the changes)

taldcroft · 2021-04-08T10:54:20Z

Yes squashing the commits would be good, maybe even down to just one.

Cleanup unused function and add test for uncovered lines

matteobachetti · 2021-04-08T11:57:16Z

@taldcroft I tried to preserve the main "building blocks" of this PR, while discarding the initial io.misc interface. It's a quite big rebase, hope it works :D

taldcroft

@matteobachetti - looks great now, thanks for all the hard work and perseverance!

github-actions Bot added the io.misc label Jan 18, 2021

matteobachetti changed the title ~~Add QDP data file handling~~ [WIP] Add QDP data file handling Jan 18, 2021

matteobachetti changed the title ~~[WIP] Add QDP data file handling~~ Add QDP data file handling Jan 18, 2021

embray suggested changes Jan 18, 2021

View reviewed changes

pllim requested a review from taldcroft January 19, 2021 17:54

pllim added Enhancement whatsnew-needed labels Jan 19, 2021

pllim added this to the v4.3 milestone Jan 19, 2021

pllim reviewed Jan 19, 2021

View reviewed changes

taldcroft reviewed Jan 21, 2021

View reviewed changes

Comment thread astropy/io/misc/tests/test_qdp.py Resolved

matteobachetti changed the title ~~Add QDP data file handling~~ [WIP] Add QDP data file handling Jan 21, 2021

matteobachetti force-pushed the add_qdp branch from 3f1fe6d to 2f401c8 Compare March 16, 2021 16:49

taldcroft reviewed Mar 17, 2021

View reviewed changes

Comment thread docs/io/unified.rst Resolved

Comment thread docs/io/unified.rst Resolved

taldcroft reviewed Mar 17, 2021

View reviewed changes

Comment thread astropy/io/ascii/tests/test_qdp.py Resolved

Comment thread astropy/io/ascii/tests/test_qdp.py Resolved

Comment thread astropy/io/ascii/tests/test_qdp.py Resolved

taldcroft reviewed Mar 18, 2021

View reviewed changes

Comment thread astropy/io/ascii/qdp.py Resolved

Comment thread astropy/io/ascii/qdp.py Resolved

Comment thread astropy/io/ascii/qdp.py Resolved

Comment thread astropy/io/ascii/qdp.py Resolved

taldcroft requested changes Apr 7, 2021

View reviewed changes

matteobachetti force-pushed the add_qdp branch from f8f8729 to a19f317 Compare April 7, 2021 19:30

matteobachetti force-pushed the add_qdp branch from 2c8402a to f2a2e3c Compare April 8, 2021 07:59

matteobachetti added 4 commits April 8, 2021 13:54

Add QDP data file handling

6553e72

Make read compatible with all formats accepted by Astropy

68d02b7

Add changelog entry for towncrier

4e3d762

Cleanup

cc7f83e

Cleanup unused function and add test for uncovered lines

matteobachetti force-pushed the add_qdp branch from 7f99207 to cc7f83e Compare April 8, 2021 11:55

taldcroft approved these changes Apr 16, 2021

View reviewed changes

taldcroft merged commit b23aa47 into astropy:main Apr 16, 2021

matteobachetti deleted the add_qdp branch April 16, 2021 12:06

taldcroft mentioned this pull request Jun 7, 2021

Allow writing of Table to cds format #11257

Closed

dhomeier mentioned this pull request Nov 3, 2023

Fix bugs when reading qdp file to allow one "NO" to start a new sector #15392

Closed

1 task

Conversation

matteobachetti commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

pep8speaks commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-04-08 11:55:44 UTC

Uh oh!

embray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

matteobachetti commented Jan 19, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

matteobachetti commented Jan 20, 2021

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matteobachetti commented Jan 21, 2021

Uh oh!

taldcroft commented Jan 21, 2021

Uh oh!

matteobachetti commented Jan 21, 2021

Uh oh!

taldcroft commented Jan 21, 2021

Uh oh!

matteobachetti commented Mar 17, 2021

Uh oh!

taldcroft commented Mar 17, 2021

Uh oh!

matteobachetti commented Mar 17, 2021

Uh oh!

taldcroft commented Mar 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matteobachetti commented Mar 17, 2021

Uh oh!

Uh oh!

Uh oh!

matteobachetti commented Mar 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft commented Mar 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matteobachetti commented Mar 18, 2021

Uh oh!

matteobachetti commented Mar 18, 2021

Uh oh!

taldcroft commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matteobachetti commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

matteobachetti commented Jan 18, 2021 •

edited

Loading

pep8speaks commented Jan 18, 2021 •

edited

Loading

taldcroft commented Mar 19, 2021 •

edited

Loading

matteobachetti commented Mar 19, 2021 •

edited

Loading