New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: SEP: Source Extractor as a library #58

Closed
whedon opened this Issue Sep 8, 2016 · 42 comments

Comments

Projects
None yet
5 participants
@whedon
Copy link
Collaborator

whedon commented Sep 8, 2016

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

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

  • 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

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v0.6.0)?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: Have any performance claims of the software been confirmed?

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

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

@whedon whedon added the review label Sep 8, 2016

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 8, 2016

/ 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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 8, 2016

I will do this one.

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 8, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 8, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 8, 2016

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

This comment has been minimized.

Copy link

kbarbary commented Sep 8, 2016

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

This comment has been minimized.

Copy link

kbarbary commented Sep 8, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 8, 2016

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

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 9, 2016

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

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 9, 2016

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

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 9, 2016

Do we need a updated paper compiling?

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 9, 2016

yes please

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 9, 2016

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 10, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 10, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 10, 2016

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

This comment has been minimized.

Copy link

jkahn commented Sep 10, 2016

@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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 10, 2016

responding in the other issue: openjournals/joss#163

@arfon arfon self-assigned this Sep 20, 2016

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 20, 2016

@whedon commands

@whedon

This comment has been minimized.

Copy link
Collaborator

whedon commented Sep 20, 2016

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

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Sep 20, 2016

@whedon assign @danielskatz as reviewer

@whedon

This comment has been minimized.

Copy link
Collaborator

whedon commented Sep 20, 2016

OK, the reviewer is @danielskatz

@arfon arfon changed the title Submission: SEP: Source Extractor as a library [REVIEW]: SEP: Source Extractor as a library Sep 20, 2016

@danielskatz danielskatz self-assigned this Sep 20, 2016

@kbarbary

This comment has been minimized.

Copy link

kbarbary commented Sep 23, 2016

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

This comment has been minimized.

Copy link

kbarbary commented Sep 23, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 24, 2016

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

This comment has been minimized.

Copy link

kbarbary commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 24, 2016

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

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 24, 2016

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 ======================

@kbarbary

This comment has been minimized.

Copy link

kbarbary commented Sep 24, 2016

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

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Sep 24, 2016

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

This comment has been minimized.

Copy link

kbarbary commented Sep 24, 2016

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

@kbarbary

This comment has been minimized.

Copy link

kbarbary commented Oct 3, 2016

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

This comment has been minimized.

Copy link
Member

arfon commented Oct 3, 2016

@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

This comment has been minimized.

Copy link

kbarbary commented Oct 4, 2016

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Oct 4, 2016

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

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Oct 4, 2016

No, I want to check this new version first.

On Oct 4, 2016, at 17:31, Arfon Smith <notifications@github.commailto:notifications@github.com> 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.

@danielskatz

This comment has been minimized.

Copy link
Collaborator

danielskatz commented Oct 5, 2016

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

This comment has been minimized.

Copy link
Member

arfon commented Oct 5, 2016

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

This comment has been minimized.

Copy link
Member

arfon commented Oct 5, 2016

@whedon commands

@whedon

This comment has been minimized.

Copy link
Collaborator

whedon commented Oct 5, 2016

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

@arfon arfon added the accepted label Oct 5, 2016

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Oct 5, 2016

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

@arfon arfon closed this Oct 5, 2016

@kbarbary

This comment has been minimized.

Copy link

kbarbary commented Oct 5, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment