search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: LATTE: Lightcurve Analysis Tool for Transiting Exoplanets #2101

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @noraeisner (N. L. Eisner) Repository: https://github.com/noraeisner/LATTE Version: v1.0.0 Editor: @xuanxu Reviewer: @christinahedges, @aureliocarnero Archive: 10.5281/zenodo.3831586

Status

status

Status badge code:

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

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) by leaving comments 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 instructions & questions

@christinahedges & @aureliocarnero, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @xuanxu know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @christinahedges

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @aureliocarnero

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

arfon commented 4 years ago

Dear authors and reviewers

We wanted to notify you that in light of the current COVID-19 pandemic, JOSS has decided to suspend submission of new manuscripts and to handle existing manuscripts (such as this one) on a "best efforts basis". We understand that you may need to attend to more pressing issues than completing a review or updating a repository in response to a review. If this is the case, a quick note indicating that you need to put a "pause" on your involvement with a review would be appreciated but is not required.

Thanks in advance for your understanding.

Arfon Smith, Editor in Chief, on behalf of the JOSS editorial team.

aureliocarnero commented 4 years ago

Hello @noraeisner, I have no capabilities at the moment to run the mpi example.... Nonetheless, we can pause now the review. Don't worry about this now. Take care and speak soon!

arfon commented 4 years ago

Hello @noraeisner, I have no capabilities at the moment to run the mpi example.... Nonetheless, we can pause now the review. Don't worry about this now. Take care and speak soon!

Or you're all welcome to continue working on this... The message above was designed to set expectations for reviewers, editors, and authors accordingly. If you all are able to keep working on this, please feel free to do so.

noraeisner commented 4 years ago

Hi all, thank you so much for your review comments and I'm so sorry for my delayed reply. It took me a couple of days to relocate (I'm in isolation in Belgium now) but I will be working on the comments today and I'll respond to the individual comments as I address them. Of course I also completely understand if the reviewers prefer to pause the process for now. I hope everyone is safe and well!

aureliocarnero commented 4 years ago

Im glad you are good! We in Spain are isolated as well now... So I'm happy to continue with the validation. Fortunately I'm healthy and I haven't been affected at all so far. Apart from Christina's comment that I agree with, I just need to look at the parallel example now. I'm happy to continue with the validation until the end.... Let me see if I can finish, my computer at home is not the best though...

aureliocarnero commented 4 years ago

Hello @noraeisner so I tested the script example_parallelized_code.py and even though I have MPI installed, I do not have more than 1 core, therefore it enters into the serial part of the code. Here, I have this error. Could you please help me?

Traceback (most recent call last):
  File "example_parallelized_code.py", line 250, in <module>
    transit_list = ast.literal_eval(((df.loc[df['TIC_ID'] == f]['db_peak']).values)[0])
  File "/home/carnero/anaconda3/envs/mypython3/lib/python3.8/site-packages/pandas/core/frame.py", line 2800, in __getitem__
    indexer = self.columns.get_loc(key)
  File "/home/carnero/anaconda3/envs/mypython3/lib/python3.8/site-packages/pandas/core/indexes/base.py", line 2648, in get_loc
    return self._engine.get_loc(self._maybe_cast_indexer(key))
  File "pandas/_libs/index.pyx", line 111, in pandas._libs.index.IndexEngine.get_loc
  File "pandas/_libs/index.pyx", line 138, in pandas._libs.index.IndexEngine.get_loc
  File "pandas/_libs/hashtable_class_helper.pxi", line 1618, in pandas._libs.hashtable.PyObjectHashTable.get_item
  File "pandas/_libs/hashtable_class_helper.pxi", line 1626, in pandas._libs.hashtable.PyObjectHashTable.get_item
KeyError: 'db_peak'
noraeisner commented 4 years ago

Hello @aureliocarnero, I'm glad to hear that you are safe and well in Spain! Belgium has now also entered a lock-down which is definitely a strange experience.

I'm really sorry, the mpi code that was on github before required the user to enter their own file structure within the code itself. I have changed it to make it much more general and it now works with the example input file - I have added a short section to the readme file of how to execute it.

I also made a small change to the GUI. If more than one sector is now identified, you can chose to look at the sectors one-by-one or all of them together. I made this change as before it was quite difficult to see what was going on if too many sectors were selected due to the figure dimensions. Nothing in the code itself has changed except the way that the interface looks.

I have also expanded on the instructions in the readme and added some more background information to the paper (as outlined here https://github.com/noraeisner/LATTE/issues/3).

aureliocarnero commented 4 years ago

@noraeisner great improvement! Im my case, we are very near the end of the validation. I'm not English speaker, so in my opinion the paper is fine, but please consider the other referee questions as well. Just a final comment, I found a bug in the example_parallelized_code.py code. Where it reads: utils.data_files(indir) utils.tp_files(indir) utils.TOI_TCE_files(indir) utils.momentum_dumps_info(indir)

I think it should read:

      LATTEutils.data_files(indir)
      LATTEutils.tp_files(indir)
      LATTEutils.TOI_TCE_files(indir)
      LATTEutils.momentum_dumps_info(indir)

Correct? Could you check that for me? And let me know when you commit the change for a final test?

arfon commented 4 years ago

:wave: all, just a friendly check-in to see how things are going with this review?

noraeisner commented 4 years ago

@aureliocarnero, thank you for seeing that bug, I have changed those lines in example_parallelized_code.py. The final version has been uploaded to Github and the pip version updated (now version 0.1.18) for whenever you have the time. I have addressed Christina comments and am waiting to hear back. Thank you so much.

xuanxu commented 4 years ago

Hi @christinahedges @aureliocarnero, hope you are safe and well. Did you have the time to check the latest changes Nora made?

aureliocarnero commented 4 years ago

Thank you for the reminder. I will check ASAP.

El mar., 12 may. 2020 a las 10:13, Juanjo Bazán (notifications@github.com) escribió:

Hi @christinahedges https://github.com/christinahedges @aureliocarnero https://github.com/aureliocarnero, hope you are safe and well. Did you have the time to check the latest changes Nora made?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2101#issuecomment-627216911, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADXSAV4HF5QG2E3ULYXUGDTRREHJ7ANCNFSM4KUYUMWQ .

aureliocarnero commented 4 years ago

So @noraeisner I found another error when running both the parallelized example and the unittests. There is an error with download and database issues...

====================================================================== ERROR: test_downloadLC (LATTE.tests.test_data_download.TestDownloadLC)

Traceback (most recent call last): File "/home/carnero/codes/LATTE/LATTE/tests/test_data_download.py", line 71, in test_downloadLC alltime, allflux, allflux_err, all_md, alltimebinned, allfluxbinned, allx1, allx2, ally1, ally2, alltimel2, allfbkg, start_sec, end_sec, in_sec, tessmag, teff, srad = LATTEutils.download_data(outdir, [5], '55525572', binfac = 5) File "/home/carnero/codes/LATTE/LATTE/LATTEutils.py", line 2639, in download_data lchdu = pf.open(response.url) # this needs to be a URL - not a file File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 150, in fitsopen return HDUList.fromfile(name, mode, memmap, save_backup, cache, File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 388, in fromfile return cls._readfrom(fileobj=fileobj, mode=mode, memmap=memmap, File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 1039, in _readfrom fileobj = _File(fileobj, mode=mode, memmap=memmap, cache=cache) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/utils/decorators.py", line 503, in wrapper return function(*args, **kwargs) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/file.py", line 147, in init self.name = download_file(fileobj, cache=cache) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/utils/data.py", line 1012, in download_file with shelve.open(urlmapfn) as url2hash: File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/shelve.py", line 243, in open return DbfilenameShelf(filename, flag, protocol, writeback) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/shelve.py", line 227, in init Shelf.init(self, dbm.open(filename, flag), protocol, writeback) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/dbm/init.py", line 91, in open raise error[0]("db type is {0}, but the module is not " dbm.error: db type is dbm.gnu, but the module is not available

Could you please check this? The same message appears when running the mpi example.... Cheers Aurelio

noraeisner commented 4 years ago

@aureliocarnero, I'm so sorry! One of the files went missing in my last git push (my computer had run out of space and started uploading random files to the cloud and off my computer). I've updated the git (unittests only work with the github version as the files that are needed for the tests are large, and it was recommended to keep the pip version as small as possible). I'm so sorry and thank you so much for you patience with this. That should solve the unittest error. If the mpi version still does not work I will remove this functionality for now, if you agree, as it works on both of my computer and I don't know enough about mpi to diagnose the problem on other operating systems (and it's not key to the code by any means). Thank you Nora

aureliocarnero commented 4 years ago

@noraeisner no problem! That is why we review it, so it ends up being a clean and neat product = ). I commit errors all the time.

So, I did a fresh installation, using pip3 install tessLATTE, and I get the error I was getting when I was doing the unittest. I can introduce a TIC ID and sectors, but then, this error message appears:

raceback (most recent call last): File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/runpy.py", line 193, in _run_module_as_main return _run_code(code, main_globals, None, File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/runpy.py", line 86, in _run_code exec(code, run_globals) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/LATTE/main.py", line 424, in utils.interact_LATTE(tic, indir, syspath, sectors_all, sectors, ra, dec, args) # the argument of whether to shos the images or not File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/LATTE/LATTEutils.py", line 129, in interact_LATTE alltime, allflux, allflux_err, all_md, alltimebinned, allfluxbinned, allx1, allx2, ally1, ally2, alltime12, allfbkg, start_sec, end_sec, in_sec, tessmag, teff, srad = download_data(indir, sectors, tic) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/LATTE/LATTEutils.py", line 2639, in download_data lchdu = pf.open(response.url) # this needs to be a URL - not a file File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 150, in fitsopen return HDUList.fromfile(name, mode, memmap, save_backup, cache, File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 388, in fromfile return cls._readfrom(fileobj=fileobj, mode=mode, memmap=memmap, File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/hdu/hdulist.py", line 1039, in _readfrom fileobj = _File(fileobj, mode=mode, memmap=memmap, cache=cache) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/utils/decorators.py", line 503, in wrapper return function(*args, **kwargs) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/io/fits/file.py", line 147, in init self.name = download_file(fileobj, cache=cache) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/site-packages/astropy/utils/data.py", line 1012, in download_file with shelve.open(urlmapfn) as url2hash: File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/shelve.py", line 243, in open return DbfilenameShelf(filename, flag, protocol, writeback) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/shelve.py", line 227, in init Shelf.init(self, dbm.open(filename, flag), protocol, writeback) File "/home/carnero/anaconda3/envs/testLATTE/lib/python3.8/dbm/init.py", line 91, in open raise error[0]("db type is {0}, but the module is not " dbm.error: db type is dbm.gnu, but the module is not available

noraeisner commented 4 years ago

Thank you! I just looked into that error message and it appears to be a problem with the astropy stored caches (https://github.com/astropy/astropy/issues/9607) so I have added the following two lines to the start of the code in LATTteutils.py:

from astropy.utils.data import clear_download_cache clear_download_cache()

I can't reproduce the same error on my end so I, unfortunately, can't check whether that works but if you could try it that would be amazing (I have updated the git version or you could copy and paste those two lines into the to the top of LATTEutils.py. Thank you

aureliocarnero commented 4 years ago

@noraeisner everything working now. Congratulations! Great work. I have validated everything and in my opinion, is ready for publication.

noraeisner commented 4 years ago

I'm so glad that that worked and thank you so much for all your feedback and help with fixing these bugs :).

Thanks, Nora

christinahedges commented 4 years ago

@noraeisner thank you for addressing my comments, the documentation looks great, and the diagnostic plots are very helpful. This is ready for publication.

xuanxu commented 4 years ago

Thank you @aureliocarnero and @christinahedges!

xuanxu commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

xuanxu commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1117/1.JATIS.1.1.014003 is OK
- 10.3847/1538-3881/aa9be0 is OK
- 10.1111/j.1365-2966.2011.19932.x is OK
- 10.1111/j.1365-2966.2004.07657.x is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1117/12.2233418 is OK
- 10.1086/667698 is OK
- 10.1093/mnras/sty2472 is OK
- 10.1051/0004-6361/201833051 is OK
- 10.1088/1538-3873/ab291c is OK

MISSING DOIs

- None

INVALID DOIs

- None
xuanxu commented 4 years ago

@noraeisner I've issued a pull request in your LATTE repo fixing some typos in the paper text. Can you take a look at it?

noraeisner commented 4 years ago

@xuanxu Thank you so much for fixing the typos. I have merged the pull request.

xuanxu commented 4 years ago

Thanks @noraeisner! Everything looks ready now. At this point could you make a new release of LATTE from current master so it includes the changes made during this review?. Then, please make an archive of the software in Zenodo (or a similar service) For the Zenodo archive make sure that:

Once you do that please report here the new version and the DOI of the archive.

noraeisner commented 4 years ago

@xuanxu I have upload the new release, v1.0.0, to Zenodo under the DOI 10.5281/zenodo.3831586. Thank you!

xuanxu commented 4 years ago

Great, thanks @noraeisner!

xuanxu commented 4 years ago

@whedon set 10.5281/zenodo.3831586 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.3831586 is the archive.

xuanxu commented 4 years ago

@whedon set v1.0.0 as version

whedon commented 4 years ago

OK. v1.0.0 is the version.

xuanxu commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

xuanxu commented 4 years ago

@whedon accept

whedon commented 4 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 4 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1448

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1448, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 4 years ago

@whedon accept deposit=true

whedon commented 4 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 4 years ago

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

whedon commented 4 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/1449
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.02101
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? notify your editorial technical team...

arfon commented 4 years ago

@christinahedges, @aureliocarnero - many thanks for your reviews here and to @xuanxu for editing this submission ✨

@noraeisner :wave: hope you are well! Your paper is now accepted into JOSS :zap::rocket::boom:

whedon commented 4 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.02101/status.svg)](https://doi.org/10.21105/joss.02101)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.02101">
  <img src="https://joss.theoj.org/papers/10.21105/joss.02101/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.02101/status.svg
   :target: https://doi.org/10.21105/joss.02101

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following: