Bugfix for issue 4781 #4782
Bugfix for issue 4781 #4782
Conversation
| if config is None: | ||
| config = {} | ||
| if config['version_1_3_or_later'] and value == '': | ||
| if config is None: config = {} |
There was a problem hiding this comment.
This change (putting the if statement on one line) is not needed - can you revert this back to 2 lines?
|
Before we can review this properly, please provide an example of a VO table that is causing these issues. We first need to be sure that this is not due to non-standard-compliant tables (which there are many of) |
|
Ok, I think I see why the if statement was incorrectly written before, but it would still be good to have an example file - since we should ideally include a regression test. |
|
Example votable attached. No, it is inline because attaching does not support XML... |
|
Validation is an interesting question. xmllint, which I trust from number of users and history, says all is good: However, it seems that astropy.io.votable.validate() disagrees: I think this is another problem but far beyond the scope of this bug and fix. I am simply pointing out you have to be careful when you say it validates as to which tool. |
| @@ -367,6 +369,8 @@ New Features | |||
|
|
|||
| - ``astropy.vo`` | |||
|
|
|||
| - KeyError when converting Table v1.2 numeric arrays fixed. [#4781] | |||
There was a problem hiding this comment.
Remove duplicate change log entry.
There was a problem hiding this comment.
Since both releases are marked as unreleased how is one to know which is
the duplicate? Obviously I can tell by the line numbers, but I am
suggesting that having 1.2 and 1.1.3 both as unreleased makes it very
unclear as to which release is going to include the pull release. Hence
I put it in both.
On Thu, 2016-04-14 at 13:30 -0700, P. L. Lim wrote:
In CHANGES.rst:
@@ -367,6 +369,8 @@ New Features
astropy.vo
- KeyError when converting Table v1.2 numeric arrays fixed. [votable.converters.NumericArray().parse() bug #4781]
Remove duplicate change log entry.
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
Al Niessner
I have never found the companion that was so companionable as solitude.
- From Walden by Henry David Thoreau
The universe is indifferent, and life is brutal; however, it is man's
choice of behavior that makes them malevolent rather than benevolent.
Some will fall in love with life and drink it from a fountain
That is pouring like an avalanche coming down the mountain.
- From the song Pepper by the Butthole Surfers
There was a problem hiding this comment.
That's why it's better to wait for a maintainer to milestone the PR before adding the changelog entry. Also please change the PR number, it should point to the PR (#4782) and not the issue it fixes.
There was a problem hiding this comment.
I did. Changeset bfc403c
On Thu, 2016-04-14 at 13:30 -0700, P. L. Lim wrote:
In CHANGES.rst:
@@ -367,6 +369,8 @@ New Features
astropy.vo
- KeyError when converting Table v1.2 numeric arrays fixed. [votable.converters.NumericArray().parse() bug #4781]
Remove duplicate change log entry.
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
Al Niessner
I have never found the companion that was so companionable as solitude.
- From Walden by Henry David Thoreau
The universe is indifferent, and life is brutal; however, it is man's
choice of behavior that makes them malevolent rather than benevolent.
Some will fall in love with life and drink it from a fountain
That is pouring like an avalanche coming down the mountain.
- From the song Pepper by the Butthole Surfers
There was a problem hiding this comment.
The workflow in your contribution howto says that you should edit the change file before doing the pull request. I thought that strange, but just used the issue number instead. I then linked that issue number with this pull request and vice versa. Seems the generating of issues, branches, and pull requests is still a bit murky. It would have been cleaner had I been able to (or knew how to?) tie this pull request to my issue. Oh well.
Now I am guessing my second email to remove the duplicate that I already had is really your @bsipocz request to change the PR. Funny, I am spending more time doing tedious changes than actually fixing code.
There was a problem hiding this comment.
@al-niessner , it's all part of the fun of open-source project contribution. 😄
|
Our VO table validation applies rules from VO standards, which extend beyond basic XML schema. I agree that the |
|
Does that really mean then that astropy.io.votable.validate() is really astropy.io.votable.verify() and that astropy.io.votable.is_votable() is equivalent to xmllint all in the context of XML? I am not suggesting name changes, I am simply trying to understand from a fixed context and I know XML while I play to votables. I appreciate that from a votable user standpoint, checking a table is called validating. |
|
@al-niessner , |
|
Once these are addressed, I think this will be okay to merge:
|
|
For test, do you want doctest or something else? I would have to put On Fri, 2016-04-15 at 08:52 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
Should be done with the requests except the testing. I hope I got the On Fri, 2016-04-15 at 08:52 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
| @@ -367,6 +367,8 @@ New Features | |||
|
|
|||
| - ``astropy.vo`` | |||
|
|
|||
| - KeyError when converting Table v1.2 numeric arrays fixed. [#4782] | |||
There was a problem hiding this comment.
Correct version section, but this needs to be in "bug fix" sub-section, not "new features".
|
For squashing commits, perhaps this tutorial will help. When it is done, you should see your changes still listed as same, but only 1 commit instead of 9 or more. As for the test, it does not need to be a doctest. The test function would reside in |
|
Is the astropy.test() sufficient for verifying my testing? Attached are There seem to be two problems really. One is that I may be have a fully I just find it disconcerting that astropy.test() is giving me the green On Mon, 2016-04-18 at 07:48 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
$ PYTHONPATH=/home/niessner/Projects/astropy/build/temp.lib.linux-x86_64-3.4:$PYTHONPATH python3
Running tests with Astropy version 1.0.3. Platform: Linux-3.13.0-85-generic-x86_64-with-Ubuntu-14.04-trusty Executable: /usr/bin/python3 Full Python Version: encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8 Numpy: 1.8.2 collected 212 items ../../usr/local/lib/python3.4/dist-packages/astropy/io/votable/tests/converter_test.py ...................... ========================== 212 passed in 1.76 seconds ========================== $ ASTROPY_USE_SYSTEM_PYTEST=1 python3 setup.py test Running tests with Astropy version 1.2.dev14912. Date: 2016-04-18T10:23:17 Platform: Linux-3.13.0-85-generic-x86_64-with-Ubuntu-14.04-trusty Executable: /usr/bin/python3 Full Python Version: encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8 Numpy: 1.8.2 collected 7691 items astropy/_erfa/tests/test_erfa.py ....... =================================== FAILURES ===================================
astropy/io/votable/tests/converter_test.py:272: source = 'data/gemini.xml', kwargs = {'table_number': 0}
astropy/io/votable/table.py:158: source = 'data/gemini.xml', columns = None, invalid = 'exception'
astropy/io/votable/table.py:138: self = <contextlib._GeneratorContextManager object at 0x7f0b16e5a9e8>
/usr/lib/python3.4/contextlib.py:59: source = 'data/gemini.xml', _debug_python_based_parser = False
astropy/utils/xml/iterparser.py:175: self = <contextlib._GeneratorContextManager object at 0x7f0b16e5a908>
/usr/lib/python3.4/contextlib.py:59: fd = 'data/gemini.xml'
astropy/utils/xml/iterparser.py:66: self = <contextlib._GeneratorContextManager object at 0x7f0b16e5a470>
/usr/lib/python3.4/contextlib.py:59: name_or_obj = 'data/gemini.xml', encoding = 'binary', cache = False
astropy/utils/data.py:199: FileNotFoundError
astropy/stats/tests/test_funcs.py:513: N = 5.0, B = 2.5, CL = 0.99
astropy/stats/funcs.py:1164: ImportError |
|
@al-niessner , hard to say because I don't know how your system is set up. To be sure, push your commit with the test and test data file addition to your PR and let Travis CI run the tests. |
|
@al-niessner - Every test sessions have a "header", listing the version numbers of both the tested package and its dependencies. Your first scenario says: This means that you're testing an installed (and may I say not up-to-date) version, and not the one you modified. Another trick: - For inserting long log outputs like above ones, I find it better to use gist (https://gist.github.com/), posting only the link keeps a clearer overview of the PRs/issues. |
|
Thanks. Enough info for me to fix all of my paths and get it working On Mon, 2016-04-18 at 11:58 -0700, Brigitta Sipocz wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
@al-niessner , I'm pretty sure the AppVeyor error will go away if you rebase, so I am not worried about that. However, are you comfortable with squashing your 11 commits into 1? If not, I'll attempt to do it via a separate PR. Except for the squashing, I think this is good to merge. Please let me know. |
|
I think I got the rebase this time. It was the push up that was causing On Tue, 2016-04-19 at 06:40 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
@al-niessner , almost there! Side effect of the squash is somehow the duplicate change log entries are back. Please remove them (again) and just leave one under 1.1.3 bug fix. No need to squash again. Thanks! |
| ''' | ||
| see Pull Request 4782 or Issue 4781 for details | ||
| ''' | ||
| table = parse_single_table (get_pkg_data_filename ('data/gemini.xml')) |
There was a problem hiding this comment.
Remove the space before the ( (twice)
|
@al-niessner - it would be great if you could squash both commits into one (and keep only the commit message for the first) |
|
I had a request not to squash from plim. So not squashing until I get a On Tue, 2016-05-10 at 03:13 -0700, Thomas Robitaille wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
Yeah, I did say that because I worry that squashing again would bring back the duplicate change log entry. But now it looks like there are 3 commits. So I am for squashing again, but we just have to make sure it does not produce unintended side effects (like the duplicate change log entry). Thanks! |
|
Done as requested. Pretty trite changes and your group would benefit On Tue, 2016-05-10 at 03:13 -0700, Thomas Robitaille wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
Trying but getting this strange error and not sure how forceful you want $ git rebase -i HEAD-3 As best I can tell from gitk the branch is just 3 straight forward On Tue, 2016-05-10 at 07:45 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
Search on google was not helpful and all suggestions ended in using I will say that it seems to be related to already having squashed the On Tue, 2016-05-10 at 07:45 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
The correct incantation is |
…ed the documentation as requested. Added a test unit and supporting data for the test. Squashed all of the commits to this single commit as requested.
|
Done and should not have odd change log problem. On Tue, 2016-05-10 at 08:10 -0700, P. L. Lim wrote:
Al Niessner I have never found the companion that was so companionable as solitude.
The universe is indifferent, and life is brutal; however, it is man's Some will fall in love with life and drink it from a fountain
|
|
Thanks @al-niessner! |
|
Looks good, thank you! |
Minor change to correct programmed failure. The first if statement checks to see if config is None and then sets it to an empty dictionary. The second conditional statement then references a non-existent key because it is an empty dictionary. Simply changed from two if statements to an ifel statement insuring that config should not be empty.