openjournals / joss-reviews

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

[REVIEW]: Mayawaves: Python Library for Interacting with the Einstein Toolkit and the MAYA Catalog #6032

Closed editorialbot closed 3 weeks ago

editorialbot commented 8 months ago

Submitting author: !--author-handle-->@deborahferguson<!--end-author-handle-- (Deborah Ferguson) Repository: https://github.com/MayaWaves/mayawaves Branch with paper.md (empty if default branch): paper Version: v2024.6 Editor: !--editor-->@eloisabentivegna<!--end-editor-- Reviewers: @cjoana, @Sbozzolo Archive: 10.5281/zenodo.11551465

Status

status

Status badge code:

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

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

@cjoana, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @Sbozzolo

📝 Checklist for @cjoana

editorialbot commented 8 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
eloisabentivegna commented 8 months ago

@editorialbot check references

editorialbot commented 8 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/1361-6382/aa7929 is OK
- 10.1103/PhysRevD.99.023003 is OK
- 10.1038/s41550-021-01568-w is OK
- 10.1088/0264-9381/32/7/074001 is OK
- 10.1088/0264-9381/32/2/024001 is OK
- 10.1093/ptep/ptaa120 is OK
- 10.1103/PhysRevLett.116.061102 is OK
- 10.1103/PhysRevX.11.021053 is OK
- 10.1103/PhysRevX.9.031040 is OK
- 10.1103/PhysRevLett.119.161101 is OK
- 10.3847/2041-8213/ab75f5 is OK
- 10.1103/PhysRevD.96.123012 is OK
- 10.1103/PhysRevLett.113.151101 is OK
- 10.1103/PhysRevD.95.044028 is OK
- 10.1103/PhysRevD.93.044007 is OK
- 10.1103/PhysRevD.96.024058 is OK
- 10.1103/PhysRevD.93.044006 is OK
- 10.1103/PhysRevD.93.122004 is OK
- 10.1103/PhysRevD.96.104041 is OK
- 10.1088/0264-9381/33/20/204001 is OK
- 10.1088/0264-9381/29/11/115001 is OK
- 10.1103/PhysRevD.76.084020 is OK
- 10.1103/PhysRevLett.103.131101 is OK
- 10.1103/PhysRevD.88.024040 is OK
- 10.1103/PhysRevD.74.104005 is OK
- 10.1103/PhysRevD.77.064032 is OK
- 10.1103/PhysRevD.77.081501 is OK
- 10.1103/PhysRevD.77.104018 is OK
- 10.1103/PhysRevD.79.084010 is OK
- 10.1103/PhysRevD.78.101503 is OK
- 10.1103/PhysRevD.91.104022 is OK
- 10.1088/0264-9381/28/19/195015 is OK
- 10.1201/b12985 is OK
- 10.21105/joss.03099 is OK

MISSING DOIs

- 10.1088/0264-9381/24/12/s04 may be a valid DOI for title: Unequal Mass Binary Black Hole Plunges and Gravitational Recoil

INVALID DOIs

- None
editorialbot commented 8 months ago
Software report:

github.com/AlDanial/cloc v 1.88  T=16.34 s (9.4 files/s, 3593.8 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                           26           3741           3104          14128
JavaScript                       14           2404           2467           9203
HTML                             15           2100             42           7725
SVG                               1              0              0           2671
Perl                              6            600            649           2508
JSON                              2              0              0           1064
Bourne Again Shell               44            230            366            932
CSS                               4            181             33            726
INI                              23             23              0            699
TeX                               1             37              0            481
Jupyter Notebook                  6              0           2059            178
Markdown                          2             24              0            100
TOML                              1              2              0             42
reStructuredText                  5             29             41             40
DOS Batch                         1              8              1             26
YAML                              1              1              4             18
make                              1              4              7              9
--------------------------------------------------------------------------------
SUM:                            153           9384           8773          40550
--------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 8 months ago

Wordcount for paper.md is 941

editorialbot commented 8 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/1361-6382/aa7929 is OK
- 10.1103/PhysRevD.99.023003 is OK
- 10.1038/s41550-021-01568-w is OK
- 10.1088/0264-9381/32/7/074001 is OK
- 10.1088/0264-9381/32/2/024001 is OK
- 10.1093/ptep/ptaa120 is OK
- 10.1103/PhysRevLett.116.061102 is OK
- 10.1103/PhysRevX.11.021053 is OK
- 10.1103/PhysRevX.9.031040 is OK
- 10.1103/PhysRevLett.119.161101 is OK
- 10.3847/2041-8213/ab75f5 is OK
- 10.1103/PhysRevD.96.123012 is OK
- 10.1103/PhysRevLett.113.151101 is OK
- 10.1103/PhysRevD.95.044028 is OK
- 10.1103/PhysRevD.93.044007 is OK
- 10.1103/PhysRevD.96.024058 is OK
- 10.1103/PhysRevD.93.044006 is OK
- 10.1103/PhysRevD.93.122004 is OK
- 10.1103/PhysRevD.96.104041 is OK
- 10.1088/0264-9381/33/20/204001 is OK
- 10.1088/0264-9381/29/11/115001 is OK
- 10.1103/PhysRevD.76.084020 is OK
- 10.1103/PhysRevLett.103.131101 is OK
- 10.1103/PhysRevD.88.024040 is OK
- 10.1103/PhysRevD.74.104005 is OK
- 10.1103/PhysRevD.77.064032 is OK
- 10.1103/PhysRevD.77.081501 is OK
- 10.1103/PhysRevD.77.104018 is OK
- 10.1103/PhysRevD.79.084010 is OK
- 10.1103/PhysRevD.78.101503 is OK
- 10.1103/PhysRevD.91.104022 is OK
- 10.1088/0264-9381/28/19/195015 is OK
- 10.1201/b12985 is OK
- 10.21105/joss.03099 is OK

MISSING DOIs

- 10.1088/0264-9381/24/12/s04 may be a valid DOI for title: Unequal Mass Binary Black Hole Plunges and Gravitational Recoil

INVALID DOIs

- None
editorialbot commented 8 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

eloisabentivegna commented 8 months ago

@deborahferguson, already a job for you: could you fix the missing DOI (see above)?

deborahferguson commented 8 months ago

Okay I just fixed it!

Sbozzolo commented 8 months ago

@eloisabentivegna can you add me as a reviewer here too? Thanks :)

eloisabentivegna commented 8 months ago

@editorialbot add @Sbozzolo as reviewer

editorialbot commented 8 months ago

@Sbozzolo added to the reviewers list!

Sbozzolo commented 8 months ago

Review checklist for @Sbozzolo

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Sbozzolo commented 7 months ago

Hi @deborahferguson, given the long time it took to find you an editor and reviewers, I already started looking at the repository and the paper. There are some important issues that would be good to address first (before the rest of the review can proceed).

My first impression is that the documentation is rather minimal (as far I can see there's only a few jupyter notebooks and the function docstrings).

Importantly:

In addition to this, it would be helpful to expand the documentation to discuss the various modules in more details, provide references/equations/conventions used, discuss features, et cetera.

Other first impressions:

The git repository is quite large (560 MB), entirely due to the files in the test folder (the code is 500 kB). It would be good to see if it is possible to reduce the size.

The git history is almost empty, so I cannot judge about authorship of the software.

Regarding the paper, it is well written. Here's some recommendations for content that should be included:

I think it would be good to mention that Einstein Toolkit is open source (is maya open source)? Given the fragmented nature of Einstein Toolkit, it would be useful to explicitly mention which thorns are supported. The paper also does not survey the state of the art regarding packages for gravitational wave analysis in Einstein Toolkit. In particular, it would be important to to discuss what additional features mayawaves has compared to the two gravitational wave analysis Python packages that come with Einstein Toolkit: kuibit (of which I am author), and POWER.

I am looking forward to reviewing your package!

deborahferguson commented 7 months ago

Thanks for the quick feedback! I'll work on adding in more documentation promptly. Thanks!

deborahferguson commented 7 months ago

@Sbozzolo In regards to the authorship, this was developed on an internal GitHub and when we decided to make it public, I moved it onto the normal GitHub. I chose not to port over all the git history due to security risks of early versions having local file paths, etc hardcoded into files (particularly in regards to tests and example simulations). That git history does still exist so maybe I can share screenshots or something of the contribution statistics

Sbozzolo commented 7 months ago

@Sbozzolo In regards to the authorship, this was developed on an internal GitHub and when we decided to make it public, I moved it onto the normal GitHub. I chose not to port over all the git history due to security risks of early versions having local file paths, etc hardcoded into files (particularly in regards to tests and example simulations). That git history does still exist so maybe I can share screenshots or something of the contribution statistics

I'll let @eloisabentivegna comment on how/if authorship should be verified. My comment was mostly to point out that I don't have much to say about it (and it is one of the boxes that need to be check).

eloisabentivegna commented 7 months ago

Good point, @Sbozzolo. Notice that non-code contributions can also be grounds for authorship in JOSS (see https://joss.readthedocs.io/en/latest/submitting.html#authorship). So, if the author list cannot clearly be established by inspecting the code repo, it is ultimately up to @deborahferguson to state who should be included (and perhaps provide a quick explanation why).

Having said this, I would like to loop in @openjournals/aass-eics to confirm.

cjoana commented 7 months ago

Review checklist for @cjoana

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

dfm commented 7 months ago

Having said this, I would like to loop in @openjournals/aass-eics to confirm.

Thanks @eloisabentivegna! I agree with everything you've said here about our authorship policy. I think that given the unique context here (missing git history) it is useful to call this out, but we're generally happy to proceed given a summary like @deborahferguson's above. Thanks all!

cjoana commented 7 months ago

Hi @deborahferguson , I am still testing these packages but so far they look great. A very useful package to analyse binary black hole mergers simulations and extract data from them. I understand that the current packages is dedicated to only black holes, but not for simulations with other type of compact objects such as neutron stars. 



I did some tests on the provided python notebooks. I understand you are currently working on expanding the documentation so I will not comment much on this, but only add some minor comments:

Overall, I found the notebooks useful and well structured to understand and a useful tool to guide on how to use these packages. I will continue testing them with alternative data from the Maya website. Regarding the paper, I also found it well written and easy to follow.

I am looking forward to the seeing the new additions on the documentation.

Sbozzolo commented 7 months ago

@deborahferguson

I learned from the comment above that I can install mayawaves with pip, so I did so. (I had to change one constraint to use python 3.11).

Now, I am trying to follow the first tutorial with some data generated with the Einstein Toolkit, but I cannot get past the first step. I've tried with a few different datasets I have, including the clean GW150914 example, and got a few different errors. In some cases, I can understand that the errors come from the fact that the data was generated with modified versions of the Einstein Toolkit and I saw in the mayawaves assume a very specific structure for files/column numbering. In other cases, I can understand that the errors come from the fact that the dataset I have on my computer is not complete or is in in a non-SIMFACTORY folder structure. For example, some of my datasets do not have the SIMFACTORY folder and par files cannot be found (even if par files exist somewhere else in the simulation output).

The GW150914 case comes directly from Einstein Toolkit, and the error is:

In [2]: mkdir /tmp/tmp3

In [3]: output = "/tmp/tmp3"

In [4]: from mayawaves.utils.postprocessingutils import create_h5_from_simulation

In [5]: simulation = "../GW150914/"

In [6]: h5_path = create_h5_from_simulation(simulation, output)
storing parameter file
storing radiative information
storing compact object information
storing metadata
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[6], line 1
----> 1 h5_path = create_h5_from_simulation(simulation, output)

File ~/Downloads/mayawaves/mayawaves/utils/postprocessingutils.py:2004, in create_h5_from_simulation(raw_directory, output_directory, catalog_id)
   2002 # process metadata
   2003 print("storing metadata")
-> 2004 _store_meta_data(h5_file, relevant_data_filepaths, relevant_output_filepaths,
   2005                  simulation_name, catalog_id)
   2007 # close h5file
   2008 h5_file.close()

File ~/Downloads/mayawaves/mayawaves/utils/postprocessingutils.py:1926, in _store_meta_data(h5_file, relevant_data_filepaths, relevant_output_filepaths, simulation_name, catalog_id)
   1924     h5_file.attrs["catalog id"] = catalog_id
   1925 # initial dimensional spins
-> 1926 dimensional_spin_0, dimensional_spin_1 = _get_initial_dimensional_spins(h5_file)
   1927 h5_file.attrs["dimensional spin 0"] = dimensional_spin_0
   1928 h5_file.attrs["dimensional spin 1"] = dimensional_spin_1

File ~/Downloads/mayawaves/mayawaves/utils/postprocessingutils.py:1541, in _get_initial_dimensional_spins(h5_file)
   1528 def _get_initial_dimensional_spins(h5_file: h5py.File) -> tuple:
   1529     """Initial dimensional spins.
   1530 
   1531     Tries to get the initial dimensional spins from the compact object data. Otherwise, returns the spins set in the
   (...)
   1539 
   1540     """
-> 1541     compact_object_data_0 = h5_file["compact_object"]["object=0"]
   1542     compact_object_data_1 = h5_file["compact_object"]["object=1"]
   1544     try:

File h5py/_objects.pyx:54, in h5py._objects.with_phil.wrapper()

File h5py/_objects.pyx:55, in h5py._objects.with_phil.wrapper()

File ~/.cache/pypoetry/virtualenvs/mayawaves-5r_dnVHS-py3.11/lib/python3.11/site-packages/h5py/_hl/group.py:357, in Group.__getitem__(self, name)
    355         raise ValueError("Invalid HDF5 object reference")
    356 elif isinstance(name, (bytes, str)):
--> 357     oid = h5o.open(self.id, self._e(name), lapl=self._lapl)
    358 else:
    359     raise TypeError("Accessing a group is done with bytes or str, "
    360                     "not {}".format(type(name)))

File h5py/_objects.pyx:54, in h5py._objects.with_phil.wrapper()

File h5py/_objects.pyx:55, in h5py._objects.with_phil.wrapper()

File h5py/h5o.pyx:189, in h5py.h5o.open()

KeyError: "Unable to synchronously open object (object 'object=0' doesn't exist)"

As you improve the documentation, it would be very useful to clearly collect and present all the requirements to use the code and the assumptions made (also for the physics).

Finally, it would be useful to wrap your HDF5 operations in a context manager to ensure that the file is properly closed when errors occurs. I had to keep restarting the repl or changing output directory everytime an error occurred because of OSError: Unable to synchronously create file (unable to truncate a file which is already open)

deborahferguson commented 7 months ago

Hi @Sbozzolo ,

Thanks for trying to get this working! I just ran it on a clean run of the GW150914 example (just one output, so not a complete run) using the pip version of mayawaves and it worked for me. I am submitting with simfactory and I now realize I've never tested to see if this works without it as our group has always used simfactory for submission. I'll take a look at what modifications need to happen for it to work without simfactory. Thanks for catching that! And yes, I'll add more documentation relating to these things. Apologies that I'm behind on that; it's application/grant season, but I've finally come back to life a bit this week.

deborahferguson commented 7 months ago

Hi @cjoana

I'm glad you're liking it so far! Yes, I am working on improving the documentation and the python version is definitely important. Unfortunately one of the libraries I'm using to perform frame conversions isn't compatible beyond python 3.10 if I recall. I'll definitely add that to the documentation. I like your solution for reducing the size! I didn't like that it was so heavy but I also wanted to be able to include a full sample simulation. Hosting it somewhere else is a great idea. Thanks for catching the typos too!

Sbozzolo commented 7 months ago

Hi @Sbozzolo ,

Thanks for trying to get this working! I just ran it on a clean run of the GW150914 example (just one output, so not a complete run) using the pip version of mayawaves and it worked for me.

Have you tried with the GW150914 data from Zenodo?

I am submitting with simfactory and I now realize I've never tested to see if this works without it as our group has always used simfactory for submission. I'll take a look at what modifications need to happen for it to work without simfactory. Thanks for catching that! And yes, I'll add more documentation relating to these things. Apologies that I'm behind on that; it's application/grant season, but I've finally come back to life a bit this week.

As a side note: I also typically use simfactory to submit my jobs, but I have a few cases where the simfactory folder is not available. For example, I often I copy the data I need on my computer for faster iteration. When I do so, I don't copy the simfactory folders. Also, when I give my simulation outputs to other people I typically restructure the folders to be simpler and contain only what is needed.

deborahferguson commented 7 months ago

Ah okay I just tried downloading it from Zenodo and I get an error as well. I'll investigate that further. For what it's worth, it shouldn't explicitly rely on having the simfactory folder as we also don't always keep that, and it's supposed to search for parfiles in the output directories. But clearly something isn't entirely working in some cases so I'll check it out.

deborahferguson commented 6 months ago

@Sbozzolo and @cjoana I've added a decent bit of additional documentation for the package, so please have a look when you have a chance (probably after the holidays of course). I'll continue iterating on it, so please let me know if anything is unclear or should be expanded upon.

deborahferguson commented 6 months ago

@Sbozzolo it seems the reason the zenodo version is failing for me is due to some python code in the GW150914.rpar file that doesn't seem compatible with recent versions of python so I'll add catches to mayawaves to ensure that doesn't cause mayawaves to fail. Have you been able to run the tutorials on the sample data included in the repo, a simulation downloaded from the MAYA catalog, or an ETK simulation you've performed?

Sbozzolo commented 5 months ago

Sorry for the delay, I was gone for the winter break.

I started looking at the new documentation, and it looks greatly improved!

A small comment: I would move "How to cite" to a seperate section instead of being the very first step in "Getting started". It is important to be easily to find, but people wont't care about it at the beginning.

Also, it would be really nice if you fixed the dependency issues that pins Python to <= 3.10. Recent versions of numpy only support >=3.9, leaving a (relatively) tight window of compatibility.

A more important comment is that I still find the capabilities of mayawaves unclear. If I follow the documentation, I read that is "an analysis library for Einstein Toolkit and MAYA simulations", but what can I do with it? The "Getting Started" page offers no help: it tells me how to the code is structured, describes some of the objects, even sketches a basic workflow, without explaning what mayawaves can do. For instance, I am assuming that mayawaves is for simulations of binary black holes only, but I cannot tell for sure. I recommend being very clear on the capabilities and the goals of the code.

I personally find having a single "features" page extremely useful. It helps people potentially interested in the package, provides discoverability, and clarifies what is supposed to be fully implemented or not.

@Sbozzolo it seems the reason the zenodo version is failing for me is due to some python code in the GW150914.rpar file that doesn't seem compatible with recent versions of python so I'll add catches to mayawaves to ensure that doesn't cause mayawaves to fail.

You can run GW150914.rpar with python3 GW150914.rpar to produce a valid par file. Then, remove the rpar from the folder. mayawaves still fails in the same way, so I am guessing that the issue is not with the rpar.

Pretty much all the simulations I have use thorns that are not supported by mayawaves (mostly the Lean codes, but also my modified version of AH/QLM codes), and getting the Zenodo data is an easy way for me to double check that mayawaves works in more a general case besides the data you provide.

deborahferguson commented 5 months ago

@Sbozzolo Ah, I see cause of that problem. In all simulations I had tested on (including the gallery examples), IO::out_dir was set to be the same as the name of the parameter file and I hadn't realized that was a parameter that could change. In the zenodo version, out_dir is not the same as the parameter file name. Thanks for finding this edge case! I'll make modifications this week to use the value of out_dir when searching for the data and will let you know once that's pushed to the main branch.

eloisabentivegna commented 4 months ago

Hi @deborahferguson, do you have updates regarding the Zenodo version?

@Sbozzolo and @cjoana: are there any other points blocking your way down the checklist?

cjoana commented 4 months ago

Hi @eloisabentivegna,

Yes, I am waiting to see if there is an update that fixes the issues loading the external data stored in Zenodo. From a previous comment by @deborahferguson, it seems this will be solved soon.

There are other minor points mentioned before that should be address:

  1. For the installation, the issue with the narrow window compatibility of python versions >=3.9 and <=3.10. If this can not be improved, at least it should be mentioned in the documentation.
  2. As suggested by @Sbozzolo, I also think that it should be indicated that the software is dedicated for the analysis of binary black holes, while for other compact objects such neutron stars the methods are not implemented yet (I believe that is the case, but not sure). 


After that, from my side, the paper will be ready to go.

deborahferguson commented 4 months ago

Hi @eloisabentivegna @Sbozzolo @cjoana

I have made changes that should make it compatible with the Zenodo version of GW150914. These changes have been merged into the main branch so you should be able to access them.

I'm currently working on reducing the size of the git repository.

Unfortunately the reason for the <=3.10 compatibility is a dependency of our library not being updated. I'll update the documentation to include a lower limit on the compatibility as well. To actually improve the compatibility to the most recent python will take time because it will require either removing a little bit of functionality or writing code from scratch to replace the library we're using. I'll also update the documentation to include that it's not yet compatible with neutron stars.

I'll update the pypi distribution once the above changes are finished.

Sbozzolo commented 4 months ago

I'll also update the documentation to include that it's not yet compatible with neutron stars.

Please note that my comment on documentation had a larger scope than just mentioning that it doesn't work with neutron stars.

Here:

A more important comment is that I still find the capabilities of mayawaves unclear. If I follow the documentation, I read that is "an analysis library for Einstein Toolkit and MAYA simulations", but what can I do with it? The "Getting Started" page offers no help: it tells me how to the code is structured, describes some of the objects, even sketches a basic workflow, without explaning what mayawaves can do. For instance, I am assuming that mayawaves is for simulations of binary black holes only, but I cannot tell for sure. I recommend being very clear on the capabilities and the goals of the code.

deborahferguson commented 4 months ago

Ah yes, thanks for that, I'll make a more detailed introduction at the beginning of the documentation to make clear the purpose of this library.

Sbozzolo commented 4 months ago

I successfully used mayawaves with the Zenodo data. The tutorials easy to follow and I found that mayawaves produces quantities that tend to roughly agree with kuibit. The code is also very well written, so my key comments are all about documentation: making it clear what mayawaves does, and making it clear how things are computed and what assumptions are being made. I had to dig thorugh the code to understand how physical quantities are calculated, but this should be easy to find in the documentation or the docstrings.

Thanks for your efforts, @deborahferguson!

eloisabentivegna commented 4 months ago

Thanks everyone for the comments and the thorough reviews.

Regarding the strict Python version requirement, it may be worth exploring other libraries providing the same functionality and compatible with later Python versions. If the library is staying stuck at 3.10, it will hold Mayawaves back and eventually make it unusable with current Python.

eloisabentivegna commented 3 months ago

Hi @Sbozzolo @cjoana, could you complete all items on your checklists, so I know you're done with the review? It feels we're not too far from completion now.

cjoana commented 3 months ago

Hi @eloisabentivegna,

Indeed, I believe we should be close from completion. I also tested loading the data from Zenodo some time ago, and works well.

@deborahferguson, you mentioned you would update the documentation to clarify the purpose of the library. Is this update still ongoing?

Sbozzolo commented 3 months ago

Hi @Sbozzolo @cjoana, could you complete all items on your checklists, so I know you're done with the review? It feels we're not too far from completion now.

As mentioned in earlier comments, I think there's need for updates in the documentation. For example, to check the box on "Functionalities", the list of available functionalities should be available and be much more granular than "post-processing".

In addition to that, the paper will also need a similar update: the paper does not describe what mayawaves does besides saying that it is a library for "processing, studying, and exporting NR simulations". It does not mention any other code that solves the same problem and how mayawaves differs from them ("Statement of need", and "State of the field"). The paper might also be a little be more specific in terms of compatibility (only a subset of Einstein Toolkit thorns are supported, only binary black holes).

eloisabentivegna commented 3 months ago

Thanks, @Sbozzolo.

@deborahferguson, could you take a look at the outstanding issues in the paper and documentation, and provide the required changes by the end of next week (April 12th)? It's only one last spring and we can complete the review process.

eloisabentivegna commented 3 months ago

@rhaas80, I'd like to loop you back in at this point. Could you comment on this submission and its suitability for JOSS? I will factor in your viewpoint in the final decision.

rhaas80 commented 3 months ago

@eloisabentivegna unfortunately there is a conflict of interest since @deborahferguson and I are currently at the same institution (and both are members of the same group) and actively collaborating on projects. So I think I am disqualified as a reviewer.

Sbozzolo commented 3 months ago

@rhaas80, I'd like to loop you back in at this point. Could you comment on this submission and its suitability for JOSS? I will factor in your viewpoint in the final decision.

My take is that the mayawaves is undoubtly suitable for JOSS. The project is not small and the code is well written. People working with the Maya codebase and catalog will certainly benefit from using it, and people working with Einstein Toolkit will have another tool at their disposal. As remarked, the documentation can and should be improved to make mayawaves more accessible.

eloisabentivegna commented 3 months ago

Thanks for clarifying, @rhaas80.

deborahferguson commented 3 months ago

@eloisabentivegna Thanks everyone for the push to get this finished. I've added some more documentation and updated the paper draft with some more details. I also made significant reductions in the file size of the directory.

Sbozzolo commented 3 months ago

@editorialbot generate pdf

editorialbot commented 3 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

Sbozzolo commented 3 months ago

Thank you @deborahferguson! You addressed most of my comments!

The main remaining ones:

deborahferguson commented 3 months ago

Thanks for your suggestions @Sbozzolo

I've updated the paper to include more citations and detail.

I've also modified the code such that users can use it with the most recent python version as long as they don't want to do center-of-mass corrections. In the case they do want to do center-of-mass corrections, it will tell them they need to downgrade to 3.10. Thanks for that suggestion!

I've also uploaded the recent updates to pypi and zenodo as a new version.