search

openjournals / joss-reviews

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

[REVIEW]: SIMsalabim: A time resolved drift-diffusion simulation software #3727

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @ljakoster (Lambert Jan Anton Koster) Repository: https://github.com/kostergroup/SIMsalabim Version: v4.29-JOSS Editor: @rkurchin Reviewer: @UTH-Tuan, @shahmoradi, @roderickmackenzie Archive: 10.5281/zenodo.6010442

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@UTH-Tuan & @shahmoradi & @roderickmackenzie, 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 @rkurchin 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 ✨

Review checklist for @UTH-Tuan

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @shahmoradi

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @roderickmackenzie

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @UTH-Tuan, @shahmoradi, @roderickmackenzie it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 3 years ago

Wordcount for paper.md is 571

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.xcrp.2021.100346 is OK
- 10.1021/acsenergylett.0c02599 is OK
- 10.1039/D0EE00714E is OK
- 10.1021/acsami.0c16411 is OK
- 10.1103/PhysRevApplied.15.014006 is OK
- 10.1021/acsenergylett.9b02720 is OK
- 10.1021/acsami.8b20493 is OK
- 10.1021/acsaem.9b00856 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.04 s (289.9 files/s, 142762.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Pascal                           7            676           1006           3334
Markdown                         3            103              0            166
TeX                              1             18              0            114
-------------------------------------------------------------------------------
SUM:                            11            797           1006           3614
-------------------------------------------------------------------------------

Statistical information for the repository 'd69dfd24522b30627402b84d' was
gathered on 2021/09/14.
No commited files with the specified extensions were found.
whedon commented 3 years ago

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

rkurchin commented 3 years ago

Copying @roderickmackenzie's comments from the pre-review issue #3412 ...

Hi There!, Thanks for your mail. Sorry for not seeing this I have a special e-mail account setup for high volume stuff which I don't check.. and this went into it.. So as far as I understand the review process, I just write comments here and then the authors respond?

I am happy to take a look but will need a few days to compile and poke at the code base. I've not used Pascal since the Borland Delphi days of 2000..

I've read the manual and had a quick poke around. Some initial comments are: Nice manual. Nice scientific description in the manual. The code seems clear on first impressions. I like the hat logo! I also like your implementation of the bernoulli function, might be faster than what I do. Exponentials are always a pain to calculate as they are done iteratively... I would be good though if you could point to literature or show a graph that what you do works. The model is clearly a nice thing and a benefit to the community. The more people are simulating these devices the better as we will have a chance of pushing renewable stuff forward...

Here are some general questions to kick off with:

Your use of the V in equations 2.1 -2.6 is a bit confusing because commonly V is only used in the DD equations when hetero-junctions are not taken into account. When the model has been adjusted to handle hetero-junctions (as yours seems to be 2.13-2.14) it's conmen to write Ec and Ev in the DD equations instead of V, to explicitly say that heterosteps can drive current. This is especially confusing as your fig 2.1 shows hetero steps so made me scratch me head as soon as I started reading until I reached the interfaces section. If you rewrite in terms of Ec and Ev you can get rid of equations 2.13-2.14, it would make the description look more like the classical DD literature as used for things like the III-V materials - and make things a bit clearer.

As I understand it there are two parts to the model a transient part and a steady state parts. In the intro you talk about transient photovoltage and AC analysis. However your equation 2.22 is the steady state form of the SRH equation, so you are assuming your trapped carrier distribution is in quasi-steady state. This means that although you can do time domain simulations your trapped carriers will always think they are in quasi-equilibrium. This won't matter for DC or for slow small perturbation stuff. But you won't for example be able to simulate things far from equilibrium, e.g. ToF transients are an extreme example of this where carriers are initially captured then gradually released from deeper and deeper traps as time goes on. TPV transients could also meet this large signal condition depending on the power of the laser pulse, and AC depending on carrier trapping/detrapping rates and the frequency you are at (and trap depth). CELIV will also fall into large perturbation regime etc... etc... I think the user should be alerted to the fact that your carriers are in SS and the model does not explicitly solve the SRH equations in time domain. Many users won't grasp this so I think it needs to be clearly pointed out and what the implications of this are for results.

I'm assuming that your trapped carriers introduced through equation 2.22 don't interact with the field? As for some devices (organics) quite a lot of the charge will be trapped (>90%) so this could affect the results. If the trapped charge does not affect the field I think the user should also be told about this in the manual. Maybe include a limitations section in the manual? Again, it depends on what the user is using the code for. If it's some perovskite device with not many traps then this may not be an issue, but if it's some ultra trappy organic thing then..

I think my main concern with points 2 and 3 is that the user knows what he/she is dealing with. There are a tonne of papers where people just use software (aka COMSOL etc..) to produce a paper without understand what is going on under the hood and results in miss interpretation of data which is not a good thing.

Would be nice if you could add some makefiles. Also would be good if you could tell me (the user) which package I need to install on Ubuntu to get pascal. I'm not super excited about installing free pascal binaries from anything but the Ubuntu repos due to security reasons. I'll install and play with over the next week then get more computer related comments back to you..

best, Rod

rkurchin commented 3 years ago

@ljakoster, I think Rod's comments above give a solid starting point for things to address on the technical side. I wanted to jump in with a slightly more "administrative" query as well:

Given that this code has been around for a long time and has suggested citations in the repo, it's important that we ensure this submission isn't in violation of JOSS's self-plagiarism policy. Can you clarify what aspects of this submission have not been previously published?

ljakoster commented 3 years ago

@rkurchin, good point. We have only published brief descriptions of the code as one does in the method-section of a paper. So, only the mathematical form of the equations and the iteration loop (simplified). The difference equations, the code, the parameters, the manual are all new.

jan anton

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Wed, 15 Sept 2021 at 15:19, Rachel Kurchin @.***> wrote:

@ljakoster https://github.com/ljakoster, I think Rod's comments above give a solid starting point for things to address on the technical side. I wanted to jump in with a slightly more "administrative" query as well:

Given that this code has been around for a long time and has suggested citations in the repo, it's important that we ensure this submission isn't in violation of JOSS's self-plagiarism policy. Can you clarify what aspects of this submission have not been previously published?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-920011842, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVD7TQBAWVGFELVZFNLUCCMOLANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ProfTuan commented 3 years ago

I am bit confused by documentation to compile it. Sorry, my previous experience was in Object Pascal using Delphi IDE. So it might be helpful to get more clarification on how to compile it. Specifically I am thrown off by the bracket placeholder ( [zimbt] )... is this supposed to be the path to the ZimbT folder?

ljakoster commented 3 years ago

We'll improve the description of how to compile the code (and where to get the compiler re Rod's comments). About ZimT, it should work by 1) installing the fpc compiler (no IDE is needed) 2) go to the ZimT folder (note: no b in spelling). 3) type fpc zimt.pas

jan anton

Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Thu, 16 Sept 2021 at 21:40, UTH-Tuan @.***> wrote:

I am bit confused by documentation to compile it. Sorry, my previous experience was in Object Pascal using Delphi IDE. So it might be helpful to get more clarification on how to compile it. Specifically I am thrown off by the bracket placeholder ( [zimbt] )... is this supposed to be the path to the ZimbT folder?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-921192267, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVGM4LH2HP3HNGA6QJLUCJBZRANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rkurchin commented 3 years ago

@shahmoradi, reminder to get your checklist started! Similarly for @UTH-Tuan, assuming the comments from @ljakoster above resolve your issues...

rkurchin commented 3 years ago

Followup ping to reviewers @UTH-Tuan @shahmoradi!

whedon commented 3 years ago

:wave: @shahmoradi, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @roderickmackenzie, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @UTH-Tuan, please update us on how your review is going (this is an automated reminder).

roderickmackenzie commented 3 years ago

@whedon Waiting for blockers from first review to be resolved.

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
rkurchin commented 3 years ago

@ljakoster, any updates on Rod's review?

@UTH-Tuan, did the response above help you to compile the code?

@shahmoradi, reminder to please start your review!

ljakoster commented 3 years ago

@rkurchin nope, I was kind of waiting for the reviews by the other reviewers so we can deal with them in 1 go. However, we will start working on Rod's comments. On a more technical note: how do I do this? do I simply address the issues, put a rebuttal somewhere and upload a new version on Github?

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Tue, 5 Oct 2021 at 14:54, Rachel Kurchin @.***> wrote:

@ljakoster https://github.com/ljakoster, any updates on Rod's review?

@UTH-Tuan https://github.com/UTH-Tuan, did the response above help you to compile the code?

@shahmoradi https://github.com/shahmoradi, reminder to please start your review!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-934384867, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVATAH7BO42TBEGRATLUFLYQXANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rkurchin commented 3 years ago

Make any updates to the code and/or manuscript that are needed to address the issues, and describe the changes here (or, if you prefer to open issues in your own repository to track things individually, that's also fine, just mention this issue in them and link to them here). If you update the manuscript, you can refresh the proof here by running @whedon generate pdf (note that this must be the first thing in your comment for Whedon to trigger properly).

shahmoradi commented 3 years ago

Comments for the authors:

rkurchin commented 3 years ago

@UTH-Tuan, were you able to run the code and begin your review?

ProfTuan commented 3 years ago

@rkurchin I have been getting errors that prevent me from running the code. Either the instructions are not clear or there is some other issue.

Mkoopm commented 3 years ago

@UTH-Tuan, if you could give me some information regarding your installation (operating system, compiler version, code version, which error message you get), I can help you debug. Just let me know.

ProfTuan commented 3 years ago

@Mkoopm, I am on macos with fpc installed (3.2.0). The error I am receiving is 

Warning: Only one source file supported, changing source file to compile from "SIMsalabim" into "ZimT"
Free Pascal Compiler version 3.2.0 [2020/05/31] for x86_64
Copyright (c) 1993-2020 by Florian Klaempfl and others
Target OS: Darwin for x86_64
Compiling ZimT
Fatal: Cannot open file "ZimT"
Fatal: Compilation aborted
Error: /usr/local/bin/ppcx64 returned an error exitcode

this is after navigting to the folder (cloned by git) and running the "fpc SIMsalabim ZimT" through the terminal.

Mkoopm commented 3 years ago

@UTH-Tuan, I see. In the main project folder, two of the subfolders are called SIMsalabim, and ZimT. To compile SIMsalabim, you should open a terminal in the SIMsalabim subfolder and run fpc SIMsalabim. To compile ZimT, you should move into the ZimT subfolder and run fpc zimt.

This is something that was not very clear in the readme, and we will upload a modified version soon to make the process a bit clearer.

ProfTuan commented 3 years ago

@Mkoopm Thanks, but now I am getting these two errors

fpc SIMsalabim

Free Pascal Compiler version 3.2.0 [2020/05/31] for x86_64
Copyright (c) 1993-2020 by Florian Klaempfl and others
Target OS: Darwin for x86_64
Compiling SIMsalabim.pas
system.pp(47,1) Fatal: Invalid PPU-File entry: 87
Fatal: Compilation aborted
Error: /usr/local/bin/ppcx64 returned an error exitcode

fpc zimt

Free Pascal Compiler version 3.2.0 [2020/05/31] for x86_64
Copyright (c) 1993-2020 by Florian Klaempfl and others
Target OS: Darwin for x86_64
Compiling zimt.pas
system.pp(45,1) Fatal: Invalid PPU-File entry: 87
Fatal: Compilation aborted
Error: /usr/local/bin/ppcx64 returned an error exitcode
Mkoopm commented 3 years ago

@UTH-Tuan, thank you for the info. It seems that your Pascal compiler is not correctly installed. This does not seem to be a problem with our codebase. I think that even a simple "Hello world!" would not compile (as long as it requires system). We have had students work with SIMsalabim on macOS before, and it compiled without problems, but we don't have any macs around to debug the compiler installation.

You could try two things:

ljakoster commented 3 years ago

@roderickmackenzie https://github.com/roderickmackenzie Review Rod MacKenzie

ANWER

We have addressed most issues raised by this reviewer, but are not yet done with some. Please find below the answers to the issues that we have addressed so far.

I've read the manual and had a quick poke around. Some initial comments are:

*Nice manual.

*Nice scientific description in the manual.

*The code seems clear on first impressions.

*I like the hat logo!

*I also like your implementation of the bernoulli function, might be faster than what I do. Exponentials are always a pain to calculate as they are done iteratively... I would be good though if you could point to literature or show a graph that what you do works.

ANSWER

We have added a short description of the approximation and a graph showing the resulting error in the reference manual (section 2.1). In doing so, we have also improved our implementation of the approximated Bernoulli function. The overall error is now smaller than 3.5E-4 (absolute), which implies that the relative error is smaller than 8E-4. This can be found in the reference manual section 2.1 and fig 2.2.

*The model is clearly a nice thing and a benefit to the community. The more people are simulating these devices the better as we will have a chance of pushing renewable stuff forward...

ANSWER

We thank the review for their nice comments. We had the logo designed (albeit on the cheap) as a token to our dedication to the software suite.

I'm assuming that your trapped carriers introduced through equation 2.22 don't interact with the field? As for some devices (organics) quite a lot of the charge will be trapped (>90%) so this could affect the results. If the trapped charge does not affect the field I think the user should also be told about this in the manual. Maybe include a limitations section in the manual? Again, it depends on what the user is using the code for. If it's some perovskite device with not many traps then this may not be an issue, but if it's some ultra trappy organic thing then..

ANSWER

Yes, all charges are taken into account in computing the field (see eq. 2.1 in reference manual). C(x) denotes any other charges that may be present, such as doping, ions, trapped charge carriers.

Would be nice if you could add some makefiles. Also would be good if you could tell me (the user) which package I need to install on Ubuntu to get pascal. I'm not super excited about installing free pascal binaries from anything but the Ubuntu repos due to security reasons.

ANSWER

We have outlined where to get fpc in the readme file. The compilation instructions are also simpler now and are reduced to a single statement, making it equally simple as a make file.

On Wed, 20 Oct 2021 at 09:23, Marten Koopmans @.***> wrote:

@UTH-Tuan https://github.com/UTH-Tuan, thank you for the info. It seems that your Pascal compiler is not correctly installed. This does not seem to be a problem with our codebase. I think that even a simple "Hello world!" would not compile (as long as it requires system). We have had students work with SIMsalabim on macOS before, and it compiled without problems, but we don't have any macs around to debug the compiler installation.

You could try two things:

  • reinstall fpc (the compiler)
  • install on Windows or Linux (it runs fine in a low-power VM, as the program requires very little resources). Using Ubuntu (20.10 or newer) the installation of the compiler is a simple one-liner, so that way you would be sure that nothing can go wrong. For ubuntu, you would use sudo apt install fpc to install the free pascal compiler.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-947398235, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVH5TSAZVULWZBKMQYLUHZU5JANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ljakoster commented 3 years ago

@shahmoradi https://github.com/shahmoradi

Reviewer Amir Shahmoradi

Comments for the authors:

-

The paper is well-written and the statement of the need is reasonable.

  A comparison with other available comparable software to SIMsalabim
  would be highly desirable.

ANSWER

We did not put a comparison in the paper as we did not want to offend anyone working in the field. Most researchers working in this field know of one another so we didn’t really feel like point out pros and cons of specific software. To name a few, SCAPS (free-ware, code not shared) does not include ionic movement, IonMonger (open-source) does not include the charge of carriers trapped at interfaces (and has a simplified expression for the SRH recombination process), Setfos (commercial) does include ionic motion but their description of interface recombination (very important in perovskite devices) is crude. All these codes have their strengths as well: Setfos has a GUI (as does SCAPS), SCAPS is much easier to use if working on established solar cells, IonMonger can handle higher ion densities.

Behind the scenes we quantitatively compared SIMsalabim (and its transient equivalent ZimT) with SCAPS and, to a lesser extent, Setfos. The agreement is excellent. The comparisons are performed by Vincent Le Corre and they can be accessed via https://github.com/VMLC-PV/Test-SIMsalabim-ZimT.

Additionally, to highlight the major features of our project (without offending our colleagues), we will add a section to the readme.md file to highlight what is so nice about our work (in addition, of course, to a section on its limitations).

-

Licensing issues:

  There are multiple CopyRight files in the project in different
  subfolders.
  Only one should really exist and if parts of the code are licensed
  differently,
  then this should be clearly stated in the front page README file
  licensing section.
  But I doubt if this is indeed the case as all license files appear to
  be either GNU or GNU Lesser.
  -

  I think it suffices to have only one set of license files at the root
  of the project.
  If the authors are worried about users' lack of attention to the
  license,
  I suggest adding the licensing information at the top of each source
  file.
  Having too many redundant files in the repository, only adds to the
  user's confusion.
  -

  My suggestion is to rename all licensing files to something that
  clearly conveys what the files indeed contain, like: LICENSE,

LICENSE.txt, LICENSE.md, ... It seems to me that JOSS requires the existence of a file named LICENSE in the root of the project, which is missing in this repository.

ANSWER

Indeed, all files are licensed under GLPL version 3. It is per GNU recommendations that we call the files ‘COPYING’ and ‘COPYING.LESSER’, see https://www.gnu.org/licenses/gpl-howto.html and we would like to stick with GNU as they wrote the licence. However, the reviewer appears to be right in saying that 1 file is enough. So we have removed all such files except the ones in the root of the project. The licensing information is described in the readme file and in Docs/Reference_Manual.pdf. Every source file points out (at the top of the file) that the project comes with a license file. However, we have not included the license text itself in every source file as that would compromise the readability of the code.

-

Binary releases:

  The binaries provided in the repository work fine for the given
  sample.
  -

  However, it seems odd and highly unusual to release binaries as part
  of the Git project. I am even surprised by the fact that GitHub

allows the uploading of binaries to a Git project hosted on GitHub. I suggest the authors upload the binaries to the release page of their project. There are currently no releases in published under this project. The binaries along with the example files should go there. Authors could separate the builds for different platforms into separate release compressed files.

ANSWER

We thank the reviewer for this excellent suggestion; we were not aware of github’s releases page. We have moved the binaries to this dedicated page and have updated the readme file accordingly. It works a charm. I know that to the more computer-savvy the inclusion of binary files seems rather odd (and I agree). However, most of the potential users of the code that we have spoken with are daunted by the prospect of having to download the fpc compiler and having to compile the code---no matter how trivial. So it seems there is a place for pre-compiled binaries.

-

Documentation:

  Missing README.md files:
  -

     Every folder should ideally contain a README.md file describing
     the reasons for the existence of the folder and a description of its
     contents. For example, what is the purpose of zimT folder? Is

it a separate project? If it is a separate project, then the authors may want to put it in a separate repository. If it is not a separate project, then its connection with SIMsalabim should be clarified.

  User Documentation:
  The User documentation looks fine on GitHub. But the file seems to be
  corrupted on Windows OS.
  The authors should check if the user documentation PDF file opens
  properly on all supported operating systems.
  It is possible that this PDF file was generated on a macOS or Linux
  OS and it is incompatible with Windows.
  In any case, it needs a fix.
  -

  change_log.txt Should likely be moved to the root of the project.
  -

  Ideally, the project would also benefit from a developer
  documentation devoted to guiding the future developers about the

structure of the code. Consider this question: If I decide to make any contributions to this project, can I start today independently, fork the project, understand the structure of the project, and make meaningful contributions to the project to merge with the main branch? This is where the developer documentation becomes handy. Right now, this project appears to become an orphan as soon as the primary contributor to the project (kostergroup) stops contributing to it, unless there is a developer guideline for the potential future developers.

ANSWER

We have updated the readme file in the root of the project to point out how the project is divided over the several subfolders (docs, units, etc); in other words, to explain the purpose of the different folders. We would much rather have a single readme file as that makes it easier to maintain the project. Please note, we are an academic group of relatively small size so this is an important issue for us. We have chosen to leave change_log.txt in the Docs folder: too many files in the root folder would likely detract from the important files such as readme.md and the licensing. To the computer-savvy this may seem like a non-issue, but most of our target audience are (early-stage) PhD students in chemistry and physics who may not realise the importance and meaning of a proper license and things like a readme.md file.

The Reference_Manual.pdf file was indeed generated on Linux but renders perfectly on our WIN 10 installations so we are unable to reproduce this problem. As explained in the Reference Manual, ZimT is the transient version of SIMsalabim and is indeed part of this project (most of the code is shared between the two programs). This should have been explained in the readme file and in the paper itself; we apologise for the oversight. We have rewritten the readme file to explain how ZimT and SIMsalabim are related. The overall project is called SIMsalabim and that is also where the main (=steady-state) program SIMsalabim gets its name from. We spent a lot of time over the past 2 years to integrate the two codes as they solve the same system of equations. Having them in 1 project makes it much easier to maintain (as we have found out over the past decade).

We followed the reviewer’s suggestion to lay down some developer guidelines: In Docs/developer_guidelines.md we have outlined how the project is structured and how it could be extended.

-

Testing and Code Coverage:

  There exists a folder named Tests in the project.
  But I could not find any code that performs any tests of the codebase
  functionality here in a verifiable manner.
  The tests in this folder resemble more a set of example input files.
  The authors should consider this question: How could a potential user
  of this library ensure that the library's functionality is verifiable at
  any time in the future? Even if the code works fine today and is accurate,
  how do we know the next person who works on this project will not
  introduce bugs and errors in the code or cause regression inadvertently?

ANSWER

The reviewer is right: the next person should be able to verify the outcomes of the code. Rather than relying on comparisons with other software (see above) we rely on physics to test the code. As a physicist, I for one find a graph that compares the numerical result with a known physical model (in a suitable limit) very compelling.

JOSS requires that there is some means of testing the code. Ideally this takes the form of automated testing. However, manual tests are also allowed and that is what we have done. The different folders (testx) indeed include the SIMsalabim or ZimT input device_parameters.txt that will generate the data. To run the test all that is required is to copy the parameter files to the SIMsalabim / ZimT folder and run either program. We have modified the file tests.md to make clear how the testing should be done (we have also pointed this out in the developer instructions).

At this stage of the project (SIMsalabim started back in 2001) it is too late in the day to introduce automated unit tests for the code. Alas, we don’t have the time to do this.

-

Compilation:

  I have tested the compilation of the two codebases on Windows
  and it seems to compile and run fine with the supplied example input
  file.

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Wed, 20 Oct 2021 at 10:06, Koster, Jan Anton @.***> wrote:

@roderickmackenzie https://github.com/roderickmackenzie Review Rod MacKenzie

ANWER

We have addressed most issues raised by this reviewer, but are not yet done with some. Please find below the answers to the issues that we have addressed so far.

I've read the manual and had a quick poke around. Some initial comments are:

*Nice manual.

*Nice scientific description in the manual.

*The code seems clear on first impressions.

*I like the hat logo!

*I also like your implementation of the bernoulli function, might be faster than what I do. Exponentials are always a pain to calculate as they are done iteratively... I would be good though if you could point to literature or show a graph that what you do works.

ANSWER

We have added a short description of the approximation and a graph showing the resulting error in the reference manual (section 2.1). In doing so, we have also improved our implementation of the approximated Bernoulli function. The overall error is now smaller than 3.5E-4 (absolute), which implies that the relative error is smaller than 8E-4. This can be found in the reference manual section 2.1 and fig 2.2.

*The model is clearly a nice thing and a benefit to the community. The more people are simulating these devices the better as we will have a chance of pushing renewable stuff forward...

ANSWER

We thank the review for their nice comments. We had the logo designed (albeit on the cheap) as a token to our dedication to the software suite.

I'm assuming that your trapped carriers introduced through equation 2.22 don't interact with the field? As for some devices (organics) quite a lot of the charge will be trapped (>90%) so this could affect the results. If the trapped charge does not affect the field I think the user should also be told about this in the manual. Maybe include a limitations section in the manual? Again, it depends on what the user is using the code for. If it's some perovskite device with not many traps then this may not be an issue, but if it's some ultra trappy organic thing then..

ANSWER

Yes, all charges are taken into account in computing the field (see eq. 2.1 in reference manual). C(x) denotes any other charges that may be present, such as doping, ions, trapped charge carriers.

Would be nice if you could add some makefiles. Also would be good if you could tell me (the user) which package I need to install on Ubuntu to get pascal. I'm not super excited about installing free pascal binaries from anything but the Ubuntu repos due to security reasons.

ANSWER

We have outlined where to get fpc in the readme file. The compilation instructions are also simpler now and are reduced to a single statement, making it equally simple as a make file.

On Wed, 20 Oct 2021 at 09:23, Marten Koopmans @.***> wrote:

@UTH-Tuan https://github.com/UTH-Tuan, thank you for the info. It seems that your Pascal compiler is not correctly installed. This does not seem to be a problem with our codebase. I think that even a simple "Hello world!" would not compile (as long as it requires system). We have had students work with SIMsalabim on macOS before, and it compiled without problems, but we don't have any macs around to debug the compiler installation.

You could try two things:

  • reinstall fpc (the compiler)
  • install on Windows or Linux (it runs fine in a low-power VM, as the program requires very little resources). Using Ubuntu (20.10 or newer) the installation of the compiler is a simple one-liner, so that way you would be sure that nothing can go wrong. For ubuntu, you would use sudo apt install fpc to install the free pascal compiler.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-947398235, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVH5TSAZVULWZBKMQYLUHZU5JANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

kostergroup commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

roderickmackenzie commented 3 years ago

@kostergroup Hi, thanks for your answer. I'm still confused about: How the charge for bulk traps in equation 2.1 i.e. C(x) links to equation 2.23. How is C(x) is calculated is not in the manual and I think the term needs breaking down term by term, please give a term for trapped electrons, trapped holes, and ions also listing the doping terms. I find C(x) confusing, maybe just spell out the terms in the equation. I don't understand this sentence "The density of trapped charges follows from the density of traps and their filling." that seems to contradict equation 2.23, as that is not what SRH does. I don't understand what rate equations you use for bulk trapping, where are they in the manual? Where is the capture cross sections, how are escape rates calculated. Equation 2.23 will only work for steady state. This sentence should not be true for SRH time domain "When electrons and holes recombine via a bulk trap (SHR recombination in the bulk), then the recombination rates for electrons and holes are equal in every grid point". As each trap should have it's own IMREF so electron and hole rates can be independent (in time domain) Can you simulate a ToF transient with an exponential distribution of tail states, place the cell in negative bias then from the resulting current transient re-extract the DoS as I did here: dx.doi.org/10.1021/jp4010828 using equation 1. This should take a couple of hours but would be an acid test of traps in your model.

ljakoster commented 3 years ago

@roderickmackenzie https://github.com/roderickmackenzie *How the charge for bulk traps in equation 2.1 i.e. C(x) links to equation 2.23. C(x) is simply the sum of all charged species at x. For bulk traps, their contribution follows from SRH statistics as described, for example, in W. Shockley, W. T. Read, Phys. Rev. 1952, 87, 835–842. Eq. 2.23 only describes the corresponding recombination rate, so it does not link to C(x).

*How is C(x) is calculated is not in the manual and I think the term needs breaking down term by term, please give a term for trapped electrons, trapped holes, and ions also listing the doping terms. I find C(x) confusing, maybe just spell out the terms in the equation.

The details of how C(x) are calculated are 1) in the code, 2) in ref. 2 in the reference manual. Please note, that our interface recombination model is an extension of the SRH work: it puts traps at an interface and charges can be captured & emitted to the conduction and valence bands on either side. Therefore, it consists of 8 rates, rather than 4. The resulting expression (and the way we linearize this in the Poisson solver) warrants a separate publication (=ref 2 in manual, in prep.). Our treatment of this process also goes beyond what is incorporated in, e.g., IonMonger as their interfacial charges do not show up in the Poisson equation and they simplify the rates.

*I don't understand this sentence "The density of trapped charges follows from the density of traps and their filling." that seems to contradict equation 2.23, as that is not what SRH does.

-This sentence expresses that there is a number of traps per volume (the density of the traps), and they will have a filling. Their product is, naturally, the density of trapped charges. What we do is exactly what SRH in their papers do for bulk traps. It is, therefore, quasi-steady-state as remarked. We are writing a section in the reference manual to point this out (i.e. the limitations and strengths of the model). Equation 2.23 does not specify the filling, but rather resulting recombination for bulk traps in steady-state.

*I don't understand what rate equations you use for bulk trapping, where are they in the manual? Where is the capture cross sections, how are escape rates calculated. Equation 2.23 will only work for steady state.

-Yes, quasi equilibrium / steady-state only, as remarked. Please note, we only replied to a fraction of your comments (as noted) as we are working on the rest. The strengths and limitations of our code will be outlined in a section in the reference manual. Future plans include moving beyond the quasi steady-state approach.

*This sentence should not be true for SRH time domain "When electrons and holes recombine via a bulk trap (SHR recombination in the bulk), then the recombination rates for electrons and holes are equal in every grid point". As each trap should have it's own IMREF so electron and hole rates can be independent (in time domain)

-see above.

*Can you simulate a ToF transient with an exponential distribution of tail states, place the cell in negative bias then from the resulting current transient re-extract the DoS as I did here: dx.doi.org/10.1021/jp4010828 using equation 1. This should take a couple of hours but would be an acid test of traps in your model.

-No, this is not possible as we do not have an exponential distribution of traps. Previous version of SIMsalabim did include such distributions (and a Gaussian one as well), but we have removed them as they were hardly used by the users of the code. A further acid test of traps in our model consists test 5.

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Wed, 20 Oct 2021 at 11:28, Roderick MacKenzie @.***> wrote:

@kostergroup https://github.com/kostergroup Hi, thanks for your answer. I'm still confused about: How the charge for bulk traps in equation 2.1 i.e. C(x) links to equation 2.23. How is C(x) is calculated is not in the manual and I think the term needs breaking down term by term, please give a term for trapped electrons, trapped holes, and ions also listing the doping terms. I find C(x) confusing, maybe just spell out the terms in the equation. I don't understand this sentence "The density of trapped charges follows from the density of traps and their filling." that seems to contradict equation 2.23, as that is not what SRH does. I don't understand what rate equations you use for bulk trapping, where are they in the manual? Where is the capture cross sections, how are escape rates calculated. Equation 2.23 will only work for steady state. This sentence should not be true for SRH time domain "When electrons and holes recombine via a bulk trap (SHR recombination in the bulk), then the recombination rates for electrons and holes are equal in every grid point". As each trap should have it's own IMREF so electron and hole rates can be independent (in time domain) Can you simulate a ToF transient with an exponential distribution of tail states, place the cell in negative bias then from the resulting current transient re-extract the DoS as I did here: dx.doi.org/10.1021/jp4010828 using equation 1. This should take a couple of hours but would be an acid test of traps in your model.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-947489944, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVELUR7J2YXYVMETARLUH2DVBANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

roderickmackenzie commented 3 years ago

Hi, @kostergroup thanks for your comments. So, I think my main concern is really the impression a reader will come away with. The abstract to the paper claims the model is a time domain and steady state i.e."SIMsalabim can do both steady-state and time-dependent simulations". Where as this is actually not true, it's more quasi-steady state model which can be run in timedomain. If you can't simulate ToF transients, then things such as CELIV and many other transient measurements where non-equlibrium traps are key are also out of reach. My worry is that a PhD student or whoever will get hold of the manual and think he/she will be understand time domain results from trappy devices. And my key worry is that this will result in papers in the literature which are misleading and not be helpful for the community. I also expect this has consequences for some perovskite devices. I think without non-equlibrium traps you should really pull back from claiming the model is time domain, in the abstract etc...

ljakoster commented 3 years ago

@roderickmackenzie I fully share your concern: it is paramount that users understand the physical model (and its limitations) that is used by software. We leave the responsibility of judging whether a model is applicable up to the user. Having said that, the user should then be able to make such a judgement. => this is one of the main reasons for us to make this open-source rather than freeware. You have raised on important point (i.e. to make clear the limitations of the model as is) and, therefore, I want to spend the time to write this up in a clear way in the reference manual. Please bear with me as I will need a bit of time to do this.

The trapping, for now, is indeed quasi-SS. Whether this is acceptable/applicable or not depends on what is being simulated, also in transient cases that include traps. After all, our approach applies as long as the time-scale of the 'experiment' is compatible with the trapping and de-trapping rates. So, things like CELIV with strong trapping are beyond approach.

An example of where transients can be applied with ZimT are JV hysteresis due to ionic movement, or transient ion drift experiments on similar devices. So, there are cases where transients can be simulated.

One of things we want to do in the near future is to include fully transient trapping / detrapping statistics. We have written the code in such a way that this should be a manageable extension. The paper, however, once accepted will remain the same; the code (and its documentation) will keep on evolving. Therefore, claims about limitations, etc. should be in the documentation (read me, reference manual) of the code. The paper, on the other hand, should refer the reader to the documentation so they can assess its suitability.

Jan Anton

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Wed, 20 Oct 2021 at 12:20, Roderick MacKenzie @.***> wrote:

Hi, @kostergroup https://github.com/kostergroup thanks for your comments. So, I think my main concern is really the impression a reader will come away with. The abstract to the paper claims the model is a time domain and steady state i.e."SIMsalabim can do both steady-state and time-dependent simulations". Where as this is actually not true, it's more quasi-steady state model which can be run in timedomain. If you can't simulate ToF transients, then things such as CELIV and many other transient measurements where non-equlibrium traps are key are also out of reach. My worry is that a PhD student or whoever will get hold of the manual and think he/she will be understand time domain results from trappy devices. And my key worry is that this will result in papers in the literature which are misleading and not be helpful for the community. I also expect this has consequences for some perovskite devices. I think without non-equlibrium traps you should really pull back from claiming the model is time domain, in the abstract etc...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-947530489, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVE5VLG7D4DAGIBNO2TUH2JXVANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Mkoopm commented 3 years ago

@UTH-Tuan, The readme and code have been updated. Amongst other things, the installation instructions have been cleared up, which might be of help to you. Binaries are available for Windows and Linux on the releases page.

ProfTuan commented 3 years ago

@UTH-Tuan, thank you for the info. It seems that your Pascal compiler is not correctly installed. This does not seem to be a problem with our codebase. I think that even a simple "Hello world!" would not compile (as long as it requires system). We have had students work with SIMsalabim on macOS before, and it compiled without problems, but we don't have any macs around to debug the compiler installation.

You could try two things:

  • reinstall fpc (the compiler)
  • install on Windows or Linux (it runs fine in a low-power VM, as the program requires very little resources). Using Ubuntu (20.10 or newer) the installation of the compiler is a simple one-liner, so that way you would be sure that nothing can go wrong. For ubuntu, you would use sudo apt install fpc to install the free pascal compiler.

@Mkoopm I don't have immediate access to Windows or Ubuntu machine, but I managed to get it compiled through installing Lazarus and using fpc installer. Previously I was using brew install so I am unsure why the former method was working and not the latter brew. I would recommend adding something about MacOS instructions for installation section.

ljakoster commented 3 years ago

@UTH-Tuan: we've adapted the readme (is now on github). Just to be clear, our installation instructions really only pertain to the installation and compilation of our software, not the fpc compiler itself.

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Sun, 24 Oct 2021 at 17:56, UTH-Tuan @.***> wrote:

@UTH-Tuan https://github.com/UTH-Tuan, thank you for the info. It seems that your Pascal compiler is not correctly installed. This does not seem to be a problem with our codebase. I think that even a simple "Hello world!" would not compile (as long as it requires system). We have had students work with SIMsalabim on macOS before, and it compiled without problems, but we don't have any macs around to debug the compiler installation.

You could try two things:

  • reinstall fpc (the compiler)
  • install on Windows or Linux (it runs fine in a low-power VM, as the program requires very little resources). Using Ubuntu (20.10 or newer) the installation of the compiler is a simple one-liner, so that way you would be sure that nothing can go wrong. For ubuntu, you would use sudo apt install fpc to install the free pascal compiler.

@Mkoopm https://github.com/Mkoopm I don't have immediate access to Windows or Ubuntu machine, but I managed to get it compiled through installing Lazarus and using fpc installer. Previously I was using brew install so I am unsure why the former method was working and not the latter brew. I would recommend adding something about MacOS instructions for installation section.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-950348970, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVCPJUUSCFOFAK5OMCTUIQUBPANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ProfTuan commented 3 years ago

@ljakoster if possible, just provide a hyperlink to https://github.com/kostergroup/SIMsalabim/blob/master/Docs/Developer_guidelines.md for the mention of it under Miscellaneous section.

ljakoster commented 3 years ago

@UTH-Tuan: thanks, that's an excellent suggestion. We're rewriting the readme file and will include the link.

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Mon, 1 Nov 2021 at 20:18, UTH-Tuan @.***> wrote:

@ljakoster https://github.com/ljakoster if possible, just provide a hyperlink to https://github.com/kostergroup/SIMsalabim/blob/master/Docs/Developer_guidelines.md for the mention of it under Miscellaneous section.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-956517643, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVHBGNUVGHTY3NSDLE3UJ3RZLANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ProfTuan commented 3 years ago

@ljakoster also this statement from above:

We did not put a comparison in the paper as we did not want to offend anyone working in the field. Most researchers working in this field know of one another so we didn’t really feel like point out pros and cons of specific software.

I don't think that's a satisfactory response. I think you can mention the other software without comparison (and without "offending" them).

ljakoster commented 3 years ago

@UTH-Tuan Yes, we can surely mention them, add a link and a short description of other software, thanks for the suggestion. I thought you were asking for a 'table' with pros and cons and features. All the codes out there have their strengths and uses and I would not feel comfortable with a table like that, especially since those codes (like ours) keep on evolving so such a table might become inaccurate/out-dated in the future. I apologize for the confusion.

best wishes, Jan-Anton

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Thu, 4 Nov 2021 at 20:36, UTH-Tuan @.***> wrote:

@ljakoster https://github.com/ljakoster also this statement from above:

We did not put a comparison in the paper as we did not want to offend anyone working in the field. Most researchers working in this field know of one another so we didn’t really feel like point out pros and cons of specific software.

I don't think that's a satisfactory response. I think you can mention the other software without comparison (and without "offending" them).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-961357220, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVCXABNHID2H7BDWM6LUKLVF7ANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rkurchin commented 3 years ago

@roderickmackenzie and @shahmoradi, are you satisfied with the responses to your most recent comments? Please do update your checklists if appropriate and leave comments/file issues for any remaining points.

roderickmackenzie commented 3 years ago

@rkurchin I believe, I am still waiting for my concerns to be answered. Happy to reassess once this has been done. I also agree with the point from @UTH-Tuan regarding citing other models which perform the same task.

shahmoradi commented 3 years ago

@kostergroup Thank you for your corrections and explanations. Given the corrections made, I have checked all boxes in my review. However, I still feel uncomfortable with not having automated tests of some sort in this library. The test results in the repository in PDf format look nice. But will they look so for the next release of the package? Who will know how to generate the test results and make appropriate e(I guess visual) comparisons to verify the accuracy of the output of the software with the next release. This becomes especially a problem given that the code for generating the plots of the test PDF files does not appear to have been shared in the repository. As I stated before, the output of the tests should be (at least semi-) automatically verified and not reply on human intervention and visual checks. The only exception is when the library's development stage is terminal and no future changes are expected to occur.

I am going to make the final decision on this particular item in agreement with other referees' opinions (@roderickmackenzie and @UTH-Tuan). All other items look fine.

@shahmoradi https://github.com/shahmoradi Reviewer Amir Shahmoradi Comments for the authors: - The paper is well-written and the statement of the need is reasonable. - A comparison with other available comparable software to SIMsalabim would be highly desirable. ANSWER We did not put a comparison in the paper as we did not want to offend anyone working in the field. Most researchers working in this field know of one another so we didn’t really feel like point out pros and cons of specific software. To name a few, SCAPS (free-ware, code not shared) does not include ionic movement, IonMonger (open-source) does not include the charge of carriers trapped at interfaces (and has a simplified expression for the SRH recombination process), Setfos (commercial) does include ionic motion but their description of interface recombination (very important in perovskite devices) is crude. All these codes have their strengths as well: Setfos has a GUI (as does SCAPS), SCAPS is much easier to use if working on established solar cells, IonMonger can handle higher ion densities. Behind the scenes we quantitatively compared SIMsalabim (and its transient equivalent ZimT) with SCAPS and, to a lesser extent, Setfos. The agreement is excellent. The comparisons are performed by Vincent Le Corre and they can be accessed via https://github.com/VMLC-PV/Test-SIMsalabim-ZimT. Additionally, to highlight the major features of our project (without offending our colleagues), we will add a section to the readme.md file to highlight what is so nice about our work (in addition, of course, to a section on its limitations). - Licensing issues: - There are multiple CopyRight files in the project in different subfolders. Only one should really exist and if parts of the code are licensed differently, then this should be clearly stated in the front page README file licensing section. But I doubt if this is indeed the case as all license files appear to be either GNU or GNU Lesser. - I think it suffices to have only one set of license files at the root of the project. If the authors are worried about users' lack of attention to the license, I suggest adding the licensing information at the top of each source file. Having too many redundant files in the repository, only adds to the user's confusion. - My suggestion is to rename all licensing files to something that clearly conveys what the files indeed contain, like: LICENSE, LICENSE.txt, LICENSE.md, ... It seems to me that JOSS requires the existence of a file named LICENSE in the root of the project, which is missing in this repository. ANSWER Indeed, all files are licensed under GLPL version 3. It is per GNU recommendations that we call the files ‘COPYING’ and ‘COPYING.LESSER’, see https://www.gnu.org/licenses/gpl-howto.html and we would like to stick with GNU as they wrote the licence. However, the reviewer appears to be right in saying that 1 file is enough. So we have removed all such files except the ones in the root of the project. The licensing information is described in the readme file and in Docs/Reference_Manual.pdf. Every source file points out (at the top of the file) that the project comes with a license file. However, we have not included the license text itself in every source file as that would compromise the readability of the code. - Binary releases: - The binaries provided in the repository work fine for the given sample. - However, it seems odd and highly unusual to release binaries as part of the Git project. I am even surprised by the fact that GitHub allows the uploading of binaries to a Git project hosted on GitHub. I suggest the authors upload the binaries to the release page of their project. There are currently no releases in published under this project. The binaries along with the example files should go there. Authors could separate the builds for different platforms into separate release compressed files. ANSWER We thank the reviewer for this excellent suggestion; we were not aware of github’s releases page. We have moved the binaries to this dedicated page and have updated the readme file accordingly. It works a charm. I know that to the more computer-savvy the inclusion of binary files seems rather odd (and I agree). However, most of the potential users of the code that we have spoken with are daunted by the prospect of having to download the fpc compiler and having to compile the code---no matter how trivial. So it seems there is a place for pre-compiled binaries. - Documentation: - Missing README.md files: - Every folder should ideally contain a README.md file describing the reasons for the existence of the folder and a description of its contents. For example, what is the purpose of zimT folder? Is it a separate project? If it is a separate project, then the authors may want to put it in a separate repository. If it is not a separate project, then its connection with SIMsalabim should be clarified. - User Documentation: The User documentation looks fine on GitHub. But the file seems to be corrupted on Windows OS. The authors should check if the user documentation PDF file opens properly on all supported operating systems. It is possible that this PDF file was generated on a macOS or Linux OS and it is incompatible with Windows. In any case, it needs a fix. - change_log.txt Should likely be moved to the root of the project. - Ideally, the project would also benefit from a developer documentation devoted to guiding the future developers about the structure of the code. Consider this question: If I decide to make any contributions to this project, can I start today independently, fork the project, understand the structure of the project, and make meaningful contributions to the project to merge with the main branch? This is where the developer documentation becomes handy. Right now, this project appears to become an orphan as soon as the primary contributor to the project (kostergroup) stops contributing to it, unless there is a developer guideline for the potential future developers. ANSWER We have updated the readme file in the root of the project to point out how the project is divided over the several subfolders (docs, units, etc); in other words, to explain the purpose of the different folders. We would much rather have a single readme file as that makes it easier to maintain the project. Please note, we are an academic group of relatively small size so this is an important issue for us. We have chosen to leave change_log.txt in the Docs folder: too many files in the root folder would likely detract from the important files such as readme.md and the licensing. To the computer-savvy this may seem like a non-issue, but most of our target audience are (early-stage) PhD students in chemistry and physics who may not realise the importance and meaning of a proper license and things like a readme.md file. The Reference_Manual.pdf file was indeed generated on Linux but renders perfectly on our WIN 10 installations so we are unable to reproduce this problem. As explained in the Reference Manual, ZimT is the transient version of SIMsalabim and is indeed part of this project (most of the code is shared between the two programs). This should have been explained in the readme file and in the paper itself; we apologise for the oversight. We have rewritten the readme file to explain how ZimT and SIMsalabim are related. The overall project is called SIMsalabim and that is also where the main (=steady-state) program SIMsalabim gets its name from. We spent a lot of time over the past 2 years to integrate the two codes as they solve the same system of equations. Having them in 1 project makes it much easier to maintain (as we have found out over the past decade). We followed the reviewer’s suggestion to lay down some developer guidelines: In Docs/developer_guidelines.md we have outlined how the project is structured and how it could be extended. - Testing and Code Coverage: - There exists a folder named Tests in the project. But I could not find any code that performs any tests of the codebase functionality here in a verifiable manner. The tests in this folder resemble more a set of example input files. The authors should consider this question: How could a potential user of this library ensure that the library's functionality is verifiable at any time in the future? Even if the code works fine today and is accurate, how do we know the next person who works on this project will not introduce bugs and errors in the code or cause regression inadvertently? ANSWER The reviewer is right: the next person should be able to verify the outcomes of the code. Rather than relying on comparisons with other software (see above) we rely on physics to test the code. As a physicist, I for one find a graph that compares the numerical result with a known physical model (in a suitable limit) very compelling. JOSS requires that there is some means of testing the code. Ideally this takes the form of automated testing. However, manual tests are also allowed and that is what we have done. The different folders (testx) indeed include the SIMsalabim or ZimT input device_parameters.txt that will generate the data. To run the test all that is required is to copy the parameter files to the SIMsalabim / ZimT folder and run either program. We have modified the file tests.md to make clear how the testing should be done (we have also pointed this out in the developer instructions). At this stage of the project (SIMsalabim started back in 2001) it is too late in the day to introduce automated unit tests for the code. Alas, we don’t have the time to do this. - Compilation: - I have tested the compilation of the two codebases on Windows and it seems to compile and run fine with the supplied example input file. … -- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl On Wed, 20 Oct 2021 at 10:06, Koster, Jan Anton @.> wrote: @roderickmackenzie https://github.com/roderickmackenzie Review Rod MacKenzie ANWER We have addressed most issues raised by this reviewer, but are not yet done with some. Please find below the answers to the issues that we have addressed so far. I've read the manual and had a quick poke around. Some initial comments are: Nice manual. Nice scientific description in the manual. The code seems clear on first impressions. I like the hat logo! I also like your implementation of the bernoulli function, might be faster than what I do. Exponentials are always a pain to calculate as they are done iteratively... I would be good though if you could point to literature or show a graph that what you do works. ANSWER We have added a short description of the approximation and a graph showing the resulting error in the reference manual (section 2.1). In doing so, we have also improved our implementation of the approximated Bernoulli function. The overall error is now smaller than 3.5E-4 (absolute), which implies that the relative error is smaller than 8E-4. This can be found in the reference manual section 2.1 and fig 2.2. The model is clearly a nice thing and a benefit to the community. The more people are simulating these devices the better as we will have a chance of pushing renewable stuff forward... ANSWER We thank the review for their nice comments. We had the logo designed (albeit on the cheap) as a token to our dedication to the software suite. I'm assuming that your trapped carriers introduced through equation 2.22 don't interact with the field? As for some devices (organics) quite a lot of the charge will be trapped (>90%) so this could affect the results. If the trapped charge does not affect the field I think the user should also be told about this in the manual. Maybe include a limitations section in the manual? Again, it depends on what the user is using the code for. If it's some perovskite device with not many traps then this may not be an issue, but if it's some ultra trappy organic thing then.. ANSWER Yes, all charges are taken into account in computing the field (see eq. 2.1 in reference manual). C(x) denotes any other charges that may be present, such as doping, ions, trapped charge carriers. Would be nice if you could add some makefiles. Also would be good if you could tell me (the user) which package I need to install on Ubuntu to get pascal. I'm not super excited about installing free pascal binaries from anything but the Ubuntu repos due to security reasons. ANSWER We have outlined where to get fpc in the readme file. The compilation instructions are also simpler now and are reduced to a single statement, making it equally simple as a make file. On Wed, 20 Oct 2021 at 09:23, Marten Koopmans @.> wrote: > @UTH-Tuan https://github.com/UTH-Tuan, thank you for the info. It > seems that your Pascal compiler is not correctly installed. This does not > seem to be a problem with our codebase. I think that even a simple "Hello > world!" would not compile (as long as it requires system). We have had > students work with SIMsalabim on macOS before, and it compiled without > problems, but we don't have any macs around to debug the compiler > installation. > > You could try two things: > > - reinstall fpc (the compiler) > - install on Windows or Linux (it runs fine in a low-power VM, as the > program requires very little resources). Using Ubuntu (20.10 or newer) the > installation of the compiler is a simple one-liner, so that way you would > be sure that nothing can go wrong. For ubuntu, you would use sudo apt > install fpc to install the free pascal compiler. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#3727 (comment)>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/AJFMMVH5TSAZVULWZBKMQYLUHZU5JANCNFSM5EAXX2FQ > . > Triage notifications on the go with GitHub Mobile for iOS > https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 > or Android > https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. > >

Mkoopm commented 3 years ago

@rkurchin: We are still working on some of the raised concerns.

@roderickmackenzie: We share your concerns about non-equilibrium traps and have made the bulk traps fully transient. We are working on the implementation of non-equilibrium interface traps and will send you an update when the changes are uploaded to GitHub.

@shahmoradi: We agree that testing is currently difficult. We are working on a python code that generates the graphs for all tests and also gives a pass/fail judgement. It should not take too long.

ljakoster commented 3 years ago

@whedon https://github.com/whedon generate pdf

-- Prof. Dr. L.J.A. (Jan Anton) Koster Zernike Institute for Advanced Materials University of Groningen open-source simulator for perovskite and organic PV https://github.com/kostergroup/SIMsalabim group website http://www.koster-group.nl

On Mon, 15 Nov 2021 at 14:15, Marten Koopmans @.***> wrote:

@rkurchin https://github.com/rkurchin: We are still working on some of the raised concerns.

@roderickmackenzie https://github.com/roderickmackenzie: We share your concerns about non-equilibrium traps and have made the bulk traps fully transient. We are working on the implementation of non-equilibrium interface traps and will send you an update when the changes are uploaded to GitHub.

@shahmoradi https://github.com/shahmoradi: We agree that testing is currently difficult. We are working on a python code that generates the graphs for all tests and also gives a pass/fail judgement. It should not take too long.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3727#issuecomment-968900110, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJFMMVFQTTBUHUQEFODZO53UMEBYVANCNFSM5EAXX2FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
kostergroup commented 3 years ago

@whedon generate pdf