Joss-reviews: [REVIEW]: SEP: Source Extractor as a library

[whedon]

Submitting author: @kbarbary (Kyle Barbary)
Repository: https://github.com/kbarbary/sep
Version: v1.0.0
Editor: @arfon
Reviewer: @danielskatz
Archive: http://dx.doi.org/10.5281/zenodo.159035

Status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/aa9bcf7c898895f5338ef5934f99061b"><img src="http://joss.theoj.org/papers/aa9bcf7c898895f5338ef5934f99061b/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/aa9bcf7c898895f5338ef5934f99061b/status.svg)](http://joss.theoj.org/papers/aa9bcf7c898895f5338ef5934f99061b)

Reviewers and authors: Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue
in the target repository and link to those issues (especially acceptance-blockers)
in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice
versa will create corresponding breadcrumb trails in the link target.)

Reviewer questions

Conflict of interest

• [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

  General checks

• [x] Repository: Is the source code for this software available at the repository url?

• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

• [x] Version: Does the release version given match the GitHub release (v0.6.0)?

  Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?

• [x] Functionality: Have the functional claims of the software been confirmed?

• [x] Performance: Have any performance claims of the software been confirmed?

  Documentation

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
  

  Software paper

Paper PDF: 10.21105.joss.00058.pdf

• [x] Authors: Does the paper.md file include a list of authors with their affiliations?
• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

[arfon]

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

• Please work through the checklist at the start of this issue.
• If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
• Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

[danielskatz]

I will do this one.

[danielskatz]

I'm a bit uncertain about the license part. The repo has a licenses directory that contains 3 licenses, two as .md and one as .txt. Is the fact that some of these are in .md ok for JORS?

Also, the readme states "The license for all parts of the code derived from SExtractor is LGPLv3. The license for code not derived from SExtractor is MIT. The license for the library as a whole is therefore LGPLv3. The license for each file is explicitly stated at the top of each file and the full text of the licenses can be found in licenses." which is fairly clear, but it's not clear to me why the licenses file then contains the contains the BSD license as well.

[danielskatz]

Re versions: The repo talks about version 1.0 as not yet released, discusses 0.60 as the most recent release, and also points to a software archive that contains version 0.30. The paper says that there is no software archive yet.

If this JOSS submission is for version 0.60, there needs to be a software archive that contains the code from version 0.60.

[danielskatz]

I feel like the original reference
Bertin, E. & Arnouts, S. 1996: SExtractor: Software for source extraction, Astronomy & Astrophysics Supplement 317, 393. (doi: 10.1051/aas:1996164)
perhaps should be included, but this is up to the submitter.

[kbarbary]

Thanks for the review!

License: I removed the copy of the BSD license and corrected the file extensions of the MIT license to txt.

versions: I updated the link in the readme to point to the v0.6.0 zenodo archive. (v1.0.0 will be the next release of SEP which will remove deprecated features; it has not yet been released though).

citation: I added the Bertin & Arnouts (1996) citation to the paper.

[kbarbary]

Ah, now I remember why the BSD license was there. One file src/overlap.h is based on BSD-licensed code. I believe this means I should keep the text of the BSD license there.

I could change MIT -> BSD to simply things overall; I just had a slight preference for MIT, so I started with that.

[danielskatz]

so the readme should be changed to say this (not the details, but just that there is some BSD code too)

[danielskatz]

@arfon, how/when does the paper get regenerated?

[arfon]

@danielskatz - I currently have to do this. @whedon is gaining those super-powers soon.

[arfon]

Do we need a updated paper compiling?

[danielskatz]

yes please

[arfon]

@danielskatz here you go: 10.21105.joss.00058.pdf

[danielskatz]

re a statement of need in the paper: the repo readme probably should have a little of the paper content to explain what source extractor is and who might want to use it. This could also potentially go in docs/index.srt

The license info in docs/index.srt might need to be updated?

It would also be useful if the docs contained a simple tutorial/test - just one input, the code needed to do an analysis, and a correct output to compare against.

[danielskatz]

re Community guidelines - the answers to the following are currently no.

Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Perhaps some more can be added to the README? or to a CONTRIBUTING file?

[danielskatz]

Installation failed on my mac:

tmp3:~ dsk$ pip install --no-deps sep
Collecting sep
Downloading sep-0.6.0.tar.gz (266kB)
100% |████████████████████████████████| 276kB 114kB/s
Installing collected packages: sep
Running setup.py install for sep ... error
Complete output from command /usr/bin/python -u -c "import setuptools, tokenize;file='/private/var/folders/bp/2p2fmtrx0w100fzhrllbvy1w0000gn/T/pip-build-DagS36/sep/setup.py';exec(compile(getattr(tokenize, 'open', open)(file).read().replace('\r\n', '\n'), file, 'exec'))" install --record /var/folders/bp/2p2fmtrx0w100fzhrllbvy1w0000gn/T/pip-BGrAfD-record/install-record.txt --single-version-externally-managed --compile:
running install
running build
running build_ext
building 'sep' extension
creating build
creating build/temp.macosx-10.11-intel-2.7
creating build/temp.macosx-10.11-intel-2.7/src

... [lots of cc's, with a fair number of warnings]

running install_lib
copying build/lib.macosx-10.11-intel-2.7/sep.so -> /Library/Python/2.7/site-packages
error: [Errno 13] Permission denied: '/Library/Python/2.7/site-packages/sep.so'

----------------------------------------

Command "/usr/bin/python -u -c "import setuptools, tokenize;file='/private/var/folders/bp/2p2fmtrx0w100fzhrllbvy1w0000gn/T/pip-build-DagS36/sep/setup.py';exec(compile(getattr(tokenize, 'open', open)(file).read().replace('\r\n', '\n'), file, 'exec'))" install --record /var/folders/bp/2p2fmtrx0w100fzhrllbvy1w0000gn/T/pip-BGrAfD-record/install-record.txt --single-version-externally-managed --compile" failed with error code 1 in /private/var/folders/bp/2p2fmtrx0w100fzhrllbvy1w0000gn/T/pip-build-DagS36/sep/

I assume this permission error is something easy to fix, but I'm unclear why others would not have the same issue, which makes me think that either there's something odd on my system (which is possible) or this is common and should be addressed in the documentation.

[jkahn]

@danielskatz this is really useful feedback, but in my role as self-appointed derail police please consider filing a ticket against the target repo bug tracker and linking to this issue instead of pasting the body of error messages into this thread.

[danielskatz]

responding in the other issue: https://github.com/openjournals/joss/issues/163

[arfon]

@whedon commands

[whedon]

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the reviewer of this submission
@whedon assign @username as reviewer

# List the GitHub usernames of the JOSS editors
@whedon list editors

# List of JOSS reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Open the review issue
@whedon start review

:construction: Important :construction:

This is all quite new. Please make sure you check the top of the issue after running a @whedon command (you might also need to refresh the page to see the issue update).

[arfon]

@whedon assign @danielskatz as reviewer

[whedon]

OK, the reviewer is @danielskatz

[kbarbary]

Thanks for the excellent feedback @danielskatz!

• Installation

I updated the install instructions in both the README and documentation. Here is the part relevant to your problem:

"If you get an error about permissions, you are probably using your system Python. In this case, I recommend using pip's "user install" option to install sep into your user directory:

pip install --no-deps --user sep

Do not install sep or other third-party Python packages using sudo unless you are fully aware of the risks."

• Contributing

I added a short paragraph on contributing to the README and docs. (It's basically just "use github.")

• Statement of need in README

I copied a bunch of the paper content into the README and the docs to provide more context, as suggested.

• License info in docs

I think this is OK as is. The docs are aimed at users, who only need to know that using sep (the Python library) is governed by LGPLv3. Developers reading the README might want to know that some files have more liberal licenses, so the README has a more detailed explanation of licenses.

• Tutorial

I added a tutorial to the docs: http://sep.readthedocs.io/en/latest/tutorial.html

[kbarbary]

Note that you should go to http://sep.readthedocs.io/en/latest to see the latest docs build. I'm planning to do a new release soon, at which point the docs changes will be visible at the default url http://sep.readthedocs.io.

[danielskatz]

Thanks - this enables me to check another box and succeed in the install.

However, there are still some issues:

./test.py gives me an error:
env: py.test: No such file or directory

the first line seems to be looking for a file that doesn't exist.

[kbarbary]

The py.test executable is provided by the pytest package, which is required to run the tests. The README does mention this requirement ("requires the pytest package"), but it could be more prominent.

[danielskatz]

I have pytest installed (via pip). Perhaps I need to do something with my path?

[danielskatz]

ok, resolved - not sure if this should be part of the docs or not.

Now that I have this working, I get 20 successes and 1 failure:

tmp3:sep dsk$ ./test.py
============================= test session starts ==============================
platform darwin -- Python 2.7.10, pytest-3.0.2, py-1.4.31, pluggy-0.3.1
rootdir: /Users/dsk/Desktop/sep, inifile:
collected 21 items

test.py F....................

=================================== FAILURES ===================================
______________________________ test_vs_sextractor ______________________________

@pytest.mark.skipif(NO_FITS, reason="no FITS reader")
def test_vs_sextractor():
    """Test behavior of sep versus sextractor.

    Note: we turn deblending off for this test. This is because the
    deblending algorithm uses a random number generator. Since the sequence
    of random numbers is not the same between sextractor and sep or between
    different platforms, object member pixels (and even the number of objects)
    can differ when deblending is on.

    Deblending is turned off by setting DEBLEND_MINCONT=1.0 in the sextractor
    configuration file and by setting deblend_cont=1.0 in sep.extract().
    """

    data = np.copy(image_data)  # make an explicit copy so we can 'subfrom'
    bkg = sep.Background(data, bw=64, bh=64, fw=3, fh=3)

    # Test that SExtractor background is same as SEP:
    bkgarr = bkg.back(dtype=np.float32)
    assert_allclose(bkgarr, image_refback, rtol=1.e-5)

        # Test that SExtractor background rms is same as SEP:
    rmsarr = bkg.rms(dtype=np.float32)

  assert_allclose(rmsarr, image_refrms, rtol=1.e-4)

test.py:90:

---

/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/numpy/testing/utils.py:1181: in assert_allclose
verbose=verbose, header=header)

---

comparison =
x = array([[-1.90392494, -1.72829401, -1.56381047, ..., -1.22583032,
-1.35....55419147, ..., -1.28184831,
-1.41667461, -1.56063807]], dtype=float32)
y = array([[ 67.97720337, 67.97009277, 67.96318817, ..., 62.05981064,
...062103, ..., 65.2692337 ,
65.27049255, 65.27170563]], dtype=float32)
err_msg = '', verbose = True
header = 'Not equal to tolerance rtol=0.0001, atol=0'

def assert_array_compare(comparison, x, y, err_msg='', verbose=True,
                         header=''):
    from numpy.core import array, isnan, isinf, any, all, inf
    x = array(x, copy=False, subok=True)
    y = array(y, copy=False, subok=True)

    def isnumber(x):
        return x.dtype.char in '?bhilqpBHILQPefdgFDG'

    def chk_same_position(x_id, y_id, hasval='nan'):
        """Handling nan/inf: check that x and y have the nan/inf at the same
        locations."""
        try:
            assert_array_equal(x_id, y_id)
        except AssertionError:
            msg = build_err_msg([x, y],
                                err_msg + '\nx and y %s location mismatch:' \
                                % (hasval), verbose=verbose, header=header,
                                names=('x', 'y'))
            raise AssertionError(msg)

    try:
        cond = (x.shape==() or y.shape==()) or x.shape == y.shape
        if not cond:
            msg = build_err_msg([x, y],
                                err_msg
                                + '\n(shapes %s, %s mismatch)' % (x.shape,
                                                                  y.shape),
                                verbose=verbose, header=header,
                                names=('x', 'y'))
            if not cond :
                raise AssertionError(msg)

        if isnumber(x) and isnumber(y):
            x_isnan, y_isnan = isnan(x), isnan(y)
            x_isinf, y_isinf = isinf(x), isinf(y)

            # Validate that the special values are in the same place
            if any(x_isnan) or any(y_isnan):
                chk_same_position(x_isnan, y_isnan, hasval='nan')
            if any(x_isinf) or any(y_isinf):
                # Check +inf and -inf separately, since they are different
                chk_same_position(x == +inf, y == +inf, hasval='+inf')
                chk_same_position(x == -inf, y == -inf, hasval='-inf')

            # Combine all the special values
            x_id, y_id = x_isnan, y_isnan
            x_id |= x_isinf
            y_id |= y_isinf

            # Only do the comparison if actual values are left
            if all(x_id):
                return

            if any(x_id):
                val = comparison(x[~x_id], y[~y_id])
            else:
                val = comparison(x, y)
        else:
            val = comparison(x, y)

        if isinstance(val, bool):
            cond = val
            reduced = [0]
        else:
            reduced = val.ravel()
            cond = reduced.all()
            reduced = reduced.tolist()
        if not cond:
            match = 100-100.0*reduced.count(1)/len(reduced)
            msg = build_err_msg([x, y],
                                err_msg
                                + '\n(mismatch %s%%)' % (match,),
                                verbose=verbose, header=header,
                                names=('x', 'y'))
            if not cond :

              raise AssertionError(msg)

E AssertionError:
E Not equal to tolerance rtol=0.0001, atol=0
E
E (mismatch 100.0%)
E x: array([[-1.90392494, -1.72829401, -1.56381047, ..., -1.22583032,
E -1.3547647 , -1.49243712],
E [-1.90392673, -1.7282958 , -1.56381226, ..., -1.22646201,...
E y: array([[ 67.97720337, 67.97009277, 67.96318817, ..., 62.05981064,
E 62.02336884, 61.98696136],
E [ 67.93690491, 67.93002319, 67.9233551 , ..., 62.09538269,...

/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/numpy/testing/utils.py:644: AssertionError
===================== 1 failed, 20 passed in 1.37 seconds ======================

[kbarbary]

This is a bug that is fixed on master (you have the master version of the tests, but the release version of the library). I'm planning to release a new version with the fix soon, but want to address any remaining issues here first.

[danielskatz]

Is it worth updating the paper for the new version?

I think that would be best, but if not, I will approve this one.

[kbarbary]

I agree. I'll do the release and post here after the paper is updated (including a new zenodo DOI).

[kbarbary]

I updated the repository (particularly codemeta.json) with the DOI for the new version (v1.0.0).

@arfon If you regenerate the paper, will it reflect the changes in codemeta.json?

[arfon]

@arfon If you regenerate the paper, will it reflect the changes in codemeta.json?

Currently no. That would be very nice if it did 😁

Would you mind adding the DOI link to this issue too?

[kbarbary]

SEP v1.0.0 DOI: http://dx.doi.org/10.5281/zenodo.159035

[arfon]

@danielskatz by the looks of the checklist I think we're good to accept here. Can you confirm?

[danielskatz]

No, I want to check this new version first.

On Oct 4, 2016, at 17:31, Arfon Smith <[email protected][e-mail]> wrote:

@danielskatzhttps://github.com/danielskatz by the looks of the checklist I think we're good to accept here. Can you confirm?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/58#issuecomment-251532835, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACx2NbKkBbMI8FOVz6pgjt5oM2_blhmRks5qwtPOgaJpZM4J3n1v.

[danielskatz]

I'm happy with the code now. I see two possible issues still, however:

1 - in the paper, I see "Software Archive: PENDING" This should be updated to http://dx.doi.org/10.5281/zenodo.159035
2 - in the metadata at the top of this issue, I see "Version: v0.6.0", which should now be 1.0.0

Otherwise, I'm happy to accept this.

[arfon]

1 - in the paper, I see "Software Archive: PENDING" This should be updated to http://dx.doi.org/10.5281/zenodo.159035
2 - in the metadata at the top of this issue, I see "Version: v0.6.0", which should now be 1.0.0

👍 I can do these two things.

Otherwise, I'm happy to accept this.

🎉

[arfon]

@whedon commands

[whedon]

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the reviewer of this submission
@whedon assign @username as reviewer

# List the GitHub usernames of the JOSS editors
@whedon list editors

# List of JOSS reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

:construction: Important :construction:

This is all quite new. Please make sure you check the top of the issue after running a @whedon command (you might also need to refresh the page to see the issue update).

[arfon]

@danielskatz many thanks for the thorough review!

@kbarbary your paper is now accepted into JOSS. Your DOI is http://dx.doi.org/10.21105/joss.00058 🎉 🚀 💥

[kbarbary]

Thanks @arfon and thanks @danielskatz for the review; it improved the package!