Closed editorialbot closed 1 year ago
editorialbot
commented
1 year 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 commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.42 s (195.8 files/s, 63754.9 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 32 1803 4452 13999
reStructuredText 30 1702 2301 403
TeX 2 7 1 127
Markdown 2 50 0 121
YAML 5 25 30 95
Jupyter Notebook 9 0 1777 74
DOS Batch 1 8 1 26
make 1 4 7 9
TOML 1 0 1 5
-------------------------------------------------------------------------------
SUM: 83 3599 8570 14859
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 930
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.23919/EuCAP51087.2021.9410981 is OK
- 10.1098/rsos.191419 is OK
MISSING DOIs
- 10.1049/icp.2022.2330 may be a valid DOI for title: Rapid Antenna and Array Analysis for Virtual Prototyping
INVALID DOIs
- None:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
arfon
commented
1 year ago @sasha-seneca – This is the review thread for the paper. All of our communications will happen here from now on.
Please read the "Reviewer instructions & questions" in the first comment above. Please create your checklist typing:
@editorialbot generate my checklistAs you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/5234 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.
sashayamadacain
commented
1 year ago sashayamadacain
commented
1 year ago Hi @arfon,
I'm very sorry, but I just realized through trying to install LyceanEM that I do not have access to any computers that have NVIDIA GPUs. From all the doucmentation I can find, it seems like this is a requirement to successfully run software that uses CUDA acceleration. For this reason, I am unable to complete my review.
The paper and documentation are generally satisfactory, but I cannot comment on the functionality or reproducibility. My only critical comment about the paper portion is that it would be helpful for the author to explicitly identify similar and/or commonly used packages related to LyceanEM. I also think that a clear statement about the proprietary nature of CUDA would be beneficial in the installation instructions (or somewhere similar). As a final note, I believe that the reach of LyceanEM would be extended if it was available in a more universal form (e.g., using OpenCL instead of CUDA); I know that this is easier said than done, but it would make the project more widely accesible.
arfon
commented
1 year ago Hi @sasha-seneca – no problem at all. Thank you so much for all of your work thus far. Are there any other items you'd recommend the author address (or issues you've opened on the upstream repository (https://github.com/LyceanEM/LyceanEM-Python))?
Also, I realise this is a big ask – but do you know anyone else who might be able to assist us with providing a second review? We've been struggling to find people with the right background (and now also with access to the appropriate hardware it would seem).
Thanks in advance!
LyceanEM
commented
1 year ago Hi @arfon @sasha-seneca
I am happy to say that although the most effective computing architecture is expected to remain CUDA, implementing CPU modes and more cross platform support is a key aspect of the planned development over the next year. I am currently preparing the vision statement for LyceanEM to lay out the roadmap for new features for the user base.
Thank you for your work on this review. If there is anything I can do to assist, please let me know.
sashayamadacain
commented
1 year ago @arfon Thank you for your understanding. I do not have any further concerns about the paper portion, nor did I open any issues in the upstream repository.
In regard to recommending a second reviewer, I am afraid that I cannot think of anyone right now.
@LyceanEM That is great news to hear. Good luck with your future endeavors!
arfon
commented
1 year ago @LyceanEM – we're definitely going to need a second reviewer here. If you have any additional suggestions (e.g., from people in your extended professional network) I'd love to hear them. If you would rather, feel free to email me on arfon.smith@gmail.com with ideas.
I will also spend more time looking for reviewers.
LyceanEM
commented
1 year ago @arfon Have you had any luck finding reviewers? I have not had much luck myself. Although perhaps @craig-warren might be willing to review, as he maintains gprMax (https://github.com/gprMax/gprMax), which has some similarities, although it is a more mature FDTD based code.
arfon
commented
1 year ago @arfon Have you had any luck finding reviewers? I have not had much luck myself.
I'm afraid I haven't sorry.
Although perhaps @craig-warren might be willing to review, as he maintains gprMax (https://github.com/gprMax/gprMax), which has some similarities, although it is a more mature FDTD based code.
Definitely could be an option. Let's see if the ping on GitHub here gets his attention, otherwise I will send him an email directly.
@craig-warren – would you be willing to review this submission for JOSS? The submission under consideration is LyceanEM, a A Python package for virtual prototyping of antenna arrays, time and frequency domain channel modelling (https://github.com/LyceanEM/LyceanEM-Python)
The review process at JOSS is unique: it takes place in a GitHub issue, is open, and author-reviewer-editor conversations are encouraged. You can learn more about the process in these guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html
Based on your experience, we think you might be able to provide a great review of this submission. Please let me know if you think you can help us out!
Many thanks Arfon
craig-warren
commented
1 year ago @arfon, yes I'd be happy to review. I've not done a JOSS review before but I see their are guidelines which I will read over in the next day or so.
LyceanEM
commented
1 year ago Thank you @craig-warren it is much appreciated.
arfon
commented
1 year ago @arfon, yes I'd be happy to review. I've not done a JOSS review before but I see their are guidelines which I will read over in the next day or so.
Wonderful, thank you so much @craig-warren. I'll go ahead and set you up as a reviewer now.
arfon
commented
1 year ago @editorialbot add @craig-warren as reviewer
editorialbot
commented
1 year ago @craig-warren added to the reviewers list!
arfon
commented
1 year ago @craig-warren – This is the review thread for the paper. All of our communications will happen here from now on.
Please read the "Reviewer instructions & questions" in the first comment above. Please create your checklist typing:
@editorialbot generate my checklistAs you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/5234 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.
arfon
commented
1 year ago Friendly reminder here @craig-warren – are you able to get started on your review soon?
craig-warren
commented
1 year ago @arfon thanks, and apologies have been swamped. Hope to start this next week.
arfon
commented
1 year ago @arfon thanks, and apologies have been swamped. Hope to start this next week.
:zap: thanks for the update @craig-warren. Please let us know if you have any questions as you get started here.
LyceanEM
commented
1 year ago Thank you for agreeing to review @craig-warren, if you have any questions about the codebase, please do let me know here or by raising an issue on the LyceanEM github and I will try to resolve it as quickly as possible.
craig-warren
commented
1 year ago craig-warren
commented
1 year ago @LyceanEM @arfon , I was finally able to start the review process today. A have a few comments (see below) but generally looks good so far. I have to find NVIDIA GPU to test functionality which I hope to do next week.
State of the field As already mentioned by @sasha-seneca, it would be useful to know what other software packages provide similar functionality. This would give the reader a better understanding of where LyceanEM fits in the EM software landscape.
References Full reference needed for ‘Rapid antenna and array analysis for virtual prototyping’ - page numbers and DOI missing.
Tests It would be useful to mention in the documentation (at a high level) the tests you have implemented using pytest
LyceanEM
commented
1 year ago @craig-warren State of the field I am not aware of any other program providing similar functionality. The motivation behind creating this software and publishing it was the identification of a need for a method for rapid virtual prototyping which is not covered by commercial solvers like CST, HFSS, FEKO etc, based upon my PhD research. The channel modelling functionality was added later.
References I have updated the paper today with all the missing DOI, when I submitted not all were avaliable. I will add the page numbers as well today.
Tests There is at the moment very limited test coverage via pytest, as I did not start development using the test driven development model. At the moment only the base uvn to xyz excitation functions, and some rotation functions are covered with automated testing. A research software engineer is joining the development team in late June, and as we move to support multiple computing architectures and other features of our development roadmap the intention is to convert fully to a test driven development process, and ideally complete test coverage.
LyceanEM
commented
1 year ago @craig-warren @arfon I have updated the paper references, and will add a statement on the state of the field shortly and will let you know when this is done.
craig-warren
commented
1 year ago @LyceanEM thanks for the responses. I am rather in the same boat (perhaps further behind!) when it comes to automated testing and gprMax. It is good to hear that you have a RSE joining the team. I think adding the sentence similar to your response on state of the field to the paper (and perhaps docs too) would be good, i.e. that LyceanEM provides a '....method for rapid virtual prototyping which is not covered by commercial solvers."
LyceanEM
commented
1 year ago @craig-warren Thanks for your comments, I have added a paragraph as discussed to the paper, together with more detail to the documentation, and a discussion of the planned development roadmap.
craig-warren
commented
1 year ago @LyceanEM I have successfully installed LyceanEM on my desktop Windows 10 machine which has external NVIDIA (1080 & 2080) GPUs attached. However, I encountered several errors when testing the examples.
Examples 01, 02, 03, 04, 05 and 08 would not fully complete with the following typical traceback:
Traceback (most recent call last):
File "01_aperture_projection.py", line 93, in <module>
directivity_envelope, pcd = aperture_projection(
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\lyceanem\models\frequency_domain.py", line 49, in aperture_projection
blocking_triangles=GF.mesh_conversion(environment)
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\lyceanem\geometry\geometryfunctions.py", line 267, in mesh_conversion
triangles = object.triangles_base_raycaster()
AttributeError: type object 'object' has no attribute 'triangles_base_raycaster'Example 06 had the following traceback:
Structure does not exist
C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\lyceanem\electromagnetics\beamforming.py:1252: RuntimeWarning: divide by zero encountered in log10
logdata = 10 * np.log10(data)
Structure does not exist
Structure does not exist
Structure does not exist
Structure does not existExample 07 had the following traceback:
Traceback (most recent call last):
File "07_aperture_farfield_polarisation.py", line 51, in <module>
Etheta, Ephi = calculate_farfield(
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\lyceanem\models\frequency_domain.py", line 147, in calculate_farfield
environment_triangles=GF.mesh_conversion(antenna_solid)
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\lyceanem\geometry\geometryfunctions.py", line 269, in mesh_conversion
exported_structure=base_classes.structures(solids=object.export_all_structures())
AttributeError: type object 'object' has no attribute 'export_all_structures'I also found I needed to install the following packages across different examples: tqdm, folium, skyfield, geopandas
LyceanEM
commented
1 year ago Hello @craig-warren
Thanks for running the examples, can you try installing lyceanem from source and seeing if this corrects the issue? I think this is due to a bug in the `mesh_conversion' which was reported and fixed. The latest code on github should run all these examples without issues, but I haven't issued a new pypi version since the bugfix.
Example 06 is correctly reporting that there is no structure to block rays provided to the raycaster, which is correct behaviour. It is a somewhat clunky message to the user currently, but it was thought preferable to flag that there are no structures.
Many Thanks
Tim
LyceanEM
commented
1 year ago Hi @craig-warren
Just as an update, I have tried the examples with the latest source code (which is the version when installing from github via pip), and all the examples run without errors, except for the warning in Example 06.
Kind Regards
Tim
craig-warren
commented
1 year ago @LyceanEM thanks, yes I managed to run all examples (except 08) successfully from the source code. Example 08 is now giving me the following traceback:
Traceback (most recent call last):
File "08_beamforming_architecture.py", line 104, in <module>
directivity_map_discrete = MaximumDirectivityMapDiscrete(
File "C:\Users\cwarren\Downloads\LyceanEM-Python\lyceanem\electromagnetics\beamforming.py", line 627, in MaximumDirectivityMapDiscrete
WS_weights = WeightTruncation(WS_weights, resolution)
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\numba\core\dispatcher.py", line 468, in _compile_for_args
error_rewrite(e, 'typing')
File "C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\numba\core\dispatcher.py", line 409, in error_rewrite
raise e.with_traceback(None)
numba.core.errors.TypingError: Failed in nopython mode pipeline (step: nopython frontend)
No implementation of function Function(<function round_ at 0x000002A201D58160>) found for signature:
>>> round_(array(complex128, 1d, C), Literal[int](0), array(float32, 1d, C))
There are 2 candidate implementations:
- Of which 2 did not match due to:
Overload in function 'impl_np_round': File: numba\np\arraymath.py: Line 3125.
With argument(s): '(array(complex128, 1d, C), int64, array(float32, 1d, C))':
Rejected as the implementation raised a specific error:
TypingError: Failed in nopython mode pipeline (step: nopython frontend)
No implementation of function Function(<built-in function setitem>) found for signature:
>>> setitem(array(float32, 1d, C), UniTuple(int64 x 1), complex128)
There are 16 candidate implementations:
- Of which 16 did not match due to:
Overload of function 'setitem': File: <numerous>: Line N/A.
With argument(s): '(array(float32, 1d, C), UniTuple(int64 x 1), complex128)':
No match.
During: typing of setitem at C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\numba\np\arraymath.py (3177)
File "..\..\miniconda3\envs\LyceanEM\lib\site-packages\numba\np\arraymath.py", line 3177:
def impl(a, decimals=0, out=None):
<source elided>
for index, val in np.ndenumerate(a):
out[index] = np.round(val, decimals)
^
raised from C:\Users\cwarren\miniconda3\envs\LyceanEM\lib\site-packages\numba\core\typeinfer.py:1086
During: resolving callee type: Function(<function round_ at 0x000002A201D58160>)
During: typing of call at C:\Users\cwarren\Downloads\LyceanEM-Python\lyceanem\electromagnetics\beamforming.py (1125)
File "..\LyceanEM-Python\lyceanem\electromagnetics\beamforming.py", line 1125:
def WeightTruncation(weights, resolution):
<source elided>
# shift numpy complex angles from -pi to pi, into 0 to 2pi, then quantise for `quant' levels
levels = np.round(Hi @craig-warren
That is more difficult to identify, the example runs fine on my platform. It looks like numba is failing to compile WeightTruncation due to variable typing not matching the candidate implementations. I can take a look at that this afternoon and issue an update to the source code? I think this has to be a numba version specific error, what version of numba are you using? In my normal environment I have numba version 0.56.4.
craig-warren
commented
1 year ago Hi @LyceanEM The version of numba was 0.57.0. I had to use Python 3.8.x to get open3d to install, it didn't work with Python 3.11.x.
LyceanEM
commented
1 year ago Hi @LyceanEM The version of numba was 0.57.0. I had to use Python 3.8.x to get open3d to install, it didn't work with Python 3.11.x.
Right, I will check with this version now. Open3d is a bit difficult as a dependancy, which is why I am moving to another backend for 3D file format support and object manipulation. I will likely then publish an optional package using open3d for visualisations.
LyceanEM
commented
1 year ago @craig-warren I have now written and tested a bugfix for this behaviour, and pushed it to the github. If you now install the latest source code it should work without error.
craig-warren
commented
1 year ago @LyceanEM I pulled the latest source and re-ran example 08. However, it seems to be just continually running on the GPU, i.e. hangs.
LyceanEM
commented
1 year ago It is an example that takes a while to run as the beamforming comparison is cpu based right now. If you reduce the farfield pattern resolution (az_res, elev_res) it should run much quicker.
LyceanEM
commented
1 year ago Also it's worth noting that the open3d windows are blocking so nothing happens until the window is closed.
craig-warren
commented
1 year ago @LyceanEM - yep got it now, reduced the farfield resolution as you suggested.
@arfon @LyceanEM I am happy and have nothing further to add to review.
LyceanEM
commented
1 year ago Thank you @craig-warren. @arfon should I now issue a new version release and store with zenodo?
arfon
commented
1 year ago @LyceanEM – At this point could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:
I can then move forward with accepting the submission.
LyceanEM
commented
1 year ago @arfon Thank you, I can confirm I have issued a new release, version 0.0.6, and archived in Zenodo with DOI 10.5281/zenodo.8026567.
arfon
commented
1 year ago @editorialbot set 10.5281/zenodo.8026567 as archive
editorialbot
commented
1 year ago Done! archive is now 10.5281/zenodo.8026567
arfon
commented
1 year ago @editorialbot recommend-accept
editorialbot
commented
1 year ago Attempting dry run of processing paper acceptance...Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.23919/EuCAP51087.2021.9410981 is OK
- 10.1109/ACCESS.2021.3074317 is OK
- 10.1098/rsos.191419 is OK
MISSING DOIs
- 10.1049/icp.2022.2330 may be a valid DOI for title: Rapid antenna and array analysis for virtual prototyping
INVALID DOIs
- None
Submitting author: !--author-handle-->@LyceanEM<!--end-author-handle-- (Timothy Pelham) Repository: https://github.com/LyceanEM/LyceanEM-Python Branch with paper.md (empty if default branch): Version: 0.04 Editor: !--editor-->@arfon<!--end-editor-- Reviewers: @sasha-seneca, @craig-warren Archive: 10.5281/zenodo.8026567
Status
Status badge code:
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
@sasha-seneca, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @arfon 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 @sasha-seneca
📝 Checklist for @craig-warren