Comments (42)
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
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
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
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.
Related Issues (20)
- [PRE REVIEW]: TRAINING AND PLACEMENT PORTAL. HOT 7
- [REVIEW]: qujax: Simulating quantum circuits with JAX HOT 10
- [REVIEW]: occupationMeasurement: A Comprehensive Toolbox for Interactive Occupation Coding in Surveys HOT 7
- [PRE REVIEW]: OasisMove: A Verified and Validated FEniCS-based Computational Fluid Dynamics Solver for Moving Domains HOT 13
- [PRE REVIEW]: NEMSEER: A Python package for downloading and handling historical National Electricity Market forecast data produced by the Australian Energy Market Operator HOT 9
- [PRE REVIEW]: pvOps: a Python package for empirical analysis of photovoltaic field data HOT 8
- [PRE REVIEW]: Time-frequency component of the Green-X library: minimax grids for efficient RPA and GW calculations HOT 31
- [REVIEW]: xinvert: A Python package for inversion problems in geophysical fluid dynamics HOT 9
- [REVIEW]: otoole: OSeMOSYS Tools for Energy Work HOT 6
- [PRE REVIEW]: DARTS: The Data Analysis Remote Treatment Service HOT 12
- [PRE REVIEW]: Triglav: Iterative Refinement and Selection of Stable Features Using Shapley Values HOT 25
- [PRE REVIEW]: CRATE: A Python package to perform fast material simulations HOT 12
- [PRE REVIEW]: GIRFReco.jl: An open-source pipeline for spiral MRI reconstruction in Julia HOT 8
- [PRE REVIEW]: Pyafscgap.org: Open source multi-modal Python-based tools for NOAA AFSC RACE GAP HOT 30
- [REVIEW]: udpPacketManager: An International LOFAR Station Data (Pre-)Processor HOT 7
- [PRE REVIEW]: COOLEST: the COde-independent Organized LEns STandard HOT 11
- [REVIEW]: The Spreadsheet Energy System Model Generator (SESMG): A tool for the optimization of urban energy systems HOT 6
- [REVIEW]: 'Shinyscreen: Mass Spectrometry Data Inspection and Quality Checking Utility HOT 6
- [PRE REVIEW]: Tethys: a Spatiotemporal Downscaling Model for Global Water Demand HOT 35
- [PRE REVIEW]: H-RISK with NoiseModelling: a QGIS plugin to predict environmental noise and estimate health risks HOT 10
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from joss-reviews.