[REVIEW]: SEP: Source Extractor as a library about joss-reviews HOT 42 CLOSED

I will do this one.

from joss-reviews.

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

from joss-reviews.

No, I want to check this new version first.

On Oct 4, 2016, at 17:31, Arfon Smith <[email protected]mailto:[email protected]> 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//issues/58#issuecomment-251532835, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACx2NbKkBbMI8FOVz6pgjt5oM2_blhmRks5qwtPOgaJpZM4J3n1v.

from joss-reviews.

/ 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! 🚀

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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

from joss-reviews.

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

from joss-reviews.

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

from joss-reviews.

Do we need a updated paper compiling?

from joss-reviews.

yes please

from joss-reviews.

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

from joss-reviews.

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.

from joss-reviews.

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?

from joss-reviews.

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.

from joss-reviews.

@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.

from joss-reviews.

responding in the other issue: openjournals/joss#163

from joss-reviews.

@whedon commands

from joss-reviews.

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

🚧 Important 🚧

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).

from joss-reviews.

@whedon assign @danielskatz as reviewer

from joss-reviews.

OK, the reviewer is @danielskatz

from joss-reviews.

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

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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.

from joss-reviews.

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

from joss-reviews.

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 = <function compare at 0x10e1b21b8>
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 ======================

from joss-reviews.

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.

from joss-reviews.

Is it worth updating the paper for the new version?

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

from joss-reviews.

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?

from joss-reviews.

@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?

from joss-reviews.

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

from joss-reviews.

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

from joss-reviews.

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.

from joss-reviews.

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.

🎉

from joss-reviews.

@whedon commands

from joss-reviews.

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

🚧 Important 🚧

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).

from joss-reviews.

@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 🎉 🚀 💥

from joss-reviews.

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

from joss-reviews.