Joss paper editorial changes

[jatkinson1000]

Here are my editorial suggestions for the paper following JOSS review in openjournals/joss-reviews#8453.

To summarise:

• The paper is currently very verbose and challenging to read in places. Several changes remove unnecessary clauses or words and intensifiers to make it easier to read.
• State that the code is specifically for fluid flow.
• Clarify what the 'examples' are later in the text (RBC and vKVS)
• Note that 'transition point' is also a term used -- since you use it yourselves in the text!
• Clarify that parallel backends are 'with minimal additional effort' rather than 'without additional effort' -- the user is still required to obtain them and specify them.
• Remove a double negative in the statement around PyNCT and symmetric positive-definite systems - please check that this is still correct.

I have a second commit that removes the section about climate modelling. The point about software maintainability is already made in the preceding paragraph and I do not see how the additional examples are relevant to this software or paper -- as far as I can tell Transiflow cannot be used with these simulations.

Not implemented here but also needing to be addressed is the statement about "Python overheads being less than 1%".
JOSS requires any performance statements to be backed up, and it is not clear to me exactly what this refers to (If I were to guess it is the overhead of running transiflow compared to running the numerical flow model in Scipy, Trilinos etc.?)
Please amend the text to be clear and either evidence the <1% or amend it to be 'small'.

[Sbte]

I unfortunately can't provide further reliable evidence to confirm the <1% claim. I did manage to produce the overhead (~8s) the Python code should have on a ~1000s solve from the baars:20 paper, but that's on a different machine. I think that's too close to 1% to be deemed reliable, especially since I have no way of doing a full run. I'll change it to small and adjust the wording.

[jatkinson1000]

OK, thanks. If you can adjust the wording and also add a reference to the baars:20 paper next to it that would be fine.

[Sbte]

@jatkinson1000 I pushed the additional changes. I also added some explanation of how we are actually using TransiFlow, which is mostly in ocean and climate modelling, hence the reference in the introduction. If this is OK, I'll merge the PR.

[jatkinson1000]

Hi @Sbte I note that you have dropped and force-pushed to overwrite history without a discussion.

I maintain that the paragraph on climate modelling is not relevant here as transiflow is not used in these papers.
Further, based on my understanding from the documentation, it is misleading to imply that transiflow as reviewed can be used with Earth System Models as a backend when the documentation states that available options are SciPy, Epetra, HYMLS, or PETSc and for rectangular domains.

I am happy with the performance wording.

[Sbte]

Hi @Sbte I note that you have dropped and force-pushed to overwrite history without a discussion.

This is a common thing to do in projects I work on, since the history is still kept in the PR, and it makes the commits that were added easier to review. If necessary, any removed commits can always be force pushed back before it's merged.

I maintain that the paragraph on climate modelling is not relevant here as transiflow is not used in these papers. Further, based on my understanding from the documentation, it is misleading to imply that transiflow as reviewed can be used with Earth System Models as a backend when the documentation states that available options are SciPy, Epetra, HYMLS, or PETSc and for rectangular domains.

It is true that in this repository we provide only the mentioned backends, but because of the modular nature, one can still use the continuation with an external discretization and/or external computational backend. The important thing is that we have a clearly defined interface that can be used in parallel computations, and that is the novelty that TransiFlow provides. The user can then easily implement their own backend and/or discretization. As mentioned in the documentation, the PETSc and Epetra backends are provided for this exact purpose. To give the user a place to start.

One of the climate use cases we have is using an external discretization with the continuation and HYMLS backend from TransiFlow, the other is using the continuation (and eigenvalue solver) from TransiFlow but has its own discretization and solver backend. The latter can be found here: https://github.com/nlesc-smcm/pop-iemic-coupling (it is imported as fvm there). POP and I-EMIC in this case both work on rectangular domains.

The discretizations implemented in TransiFlow itself are mostly there for educational purposes. For any real world simulations, I'd expect the user to implement their own, or at least amend the provided ones.

[jatkinson1000]

Hi @Sbte I'm afraid I am going to stand on this.

The JOSS paper review is for the software as openly published and publicly available.
It does not cover closed source capabilities or unpublished extensions.

Looking at the online documentation I cannot see anywhere a description or guidance of how the code can be used with arbitrary backends such as a third-party Earth system model which is what you are implying here.
The interfaces all expect a expect a Cartesian domain, and the documentation on Adding a custom model refers to changing the PDE, boundary conditions, or discretisation within this domain. It does not provide guidance on coupling a completely external model.

Note the current focus of our discussion has diverged from my original objection which was that the reference to climate modelling was not in line with the rest of the paper, superfluous to the points already made, and that transiflow was not used in the referenced works.

If you are happy to accept my original suggestion I see no further barriers to publication.

[Sbte]

Hi @Sbte I'm afraid I am going to stand on this.

The JOSS paper review is for the software as openly published and publicly available. It does not cover closed source capabilities or unpublished extensions.

All of these extensions are public. The backend for the project I mentioned earlier is implemented here https://github.com/omuse-geoscience/omuse/blob/master/src/omuse/community/iemic/interface.py and is instantiated here https://github.com/nlesc-smcm/pop-iemic-coupling/blob/main/iemic.py#L191. The EMIC itself is available here https://github.com/nlesc-smcm/i-emic/. I'd add a reference to that backend in the documentation, but that's not very high-quality code, so I don't want anyone to get confused by it.

Looking at the online documentation I cannot see anywhere a description or guidance of how the code can be used with arbitrary backends such as a third-party Earth system model which is what you are implying here. The interfaces all expect a expect a Cartesian domain, and the documentation on Adding a custom model refers to changing the PDE, boundary conditions, or discretisation within this domain. It does not provide guidance on coupling a completely external model.

That's a good point. I now added that in #63.

Note the current focus of our discussion has diverged from my original objection which was that the reference to climate modelling was not in line with the rest of the paper, superfluous to the points already made, and that transiflow was not used in the referenced works.

The reason for the reference to climate modelling is that this was the original inspiration for developing this Python package and this is where we've been using this package for the past 5 years. The package had predecessors in Fortran and C++, which have been in use for 30+ years, but we ran into serious issues with those, first after writing a hybrid Fortran/C++ package for thies:09 and then later with a more C++-oriented approach for mulder:21. This made us decide to finally completely rewrite everything. I can remove the two citations, if that's an issue, but I'd prefer to leave in the textual reference to climate modelling, since that's where all of this is coming from. I could also elaborate on that in the paper if that's preferred.

[Sbte]

@jatkinson1000 Anything I can do to make this move forward?

[jatkinson1000]

My sincere apologies for the delay @Sbte I have been on extended leave but am due to return to work next week.

Prior to this I was discussing how to proceed with the wider editorial team regarding the additional functionality claims added in #63

My fellow editors had a range of opinions as to the most appropriate approach.
Things were complicated by the fact that the work was performed using a predecessor to this code, rather than TransiFlow itself, and that the documentation for this functionality was not in the version of the code "as reviewed".

To confirm - my understanding is that coupling to an arbitrary codes to perform a stability analysis is a capability of TransiFlow as was reviewed, it was just not detailed?
I feel that this can be regarded as a review issue - improving documentation as part of the process, but it is complicated by the fact that it arose during the editorial review after the reviewers completed their checklists.
It would be good if the documentation introduced in #63 could point to some of the examples listed above so that users could see how they might couple to their own codes.

Given the functionality detailed in #63 is key to some of the details you want to leave in the paper (use of Transiflow and its predecessors with ESMs) I think the best compromise is to check with the reviewers if they feel that the functionality was clear from their initial review and from reading #63.
I have asked this over in openjournals/joss-reviews#8453

Please merge this PR and I will proceed with the remainder of the editorial checks when I return to work next week.

[Sbte]

My sincere apologies for the delay @Sbte I have been on extended leave but am due to return to work next week.

No problem!

Prior to this I was discussing how to proceed with the wider editorial team regarding the additional functionality claims added in #63

My fellow editors had a range of opinions as to the most appropriate approach. Things were complicated by the fact that the work was performed using a predecessor to this code, rather than TransiFlow itself, and that the documentation for this functionality was not in the version of the code "as reviewed".

To confirm - my understanding is that coupling to an arbitrary codes to perform a stability analysis is a capability of TransiFlow as was reviewed, it was just not detailed? I feel that this can be regarded as a review issue - improving documentation as part of the process, but it is complicated by the fact that it arose during the editorial review after the reviewers completed their checklists. It would be good if the documentation introduced in #63 could point to some of the examples listed above so that users could see how they might couple to their own codes.

Yes, it was actually detailed in the documentation of the classes that can be extended to allow for using arbitrary code with TransiFlow, but it just wasn't easy to find for new users, as was apparent from your review. For this reason, I added a new page to the documentation.

Given the functionality detailed in #63 is key to some of the details you want to leave in the paper (use of Transiflow and its predecessors with ESMs) I think the best compromise is to check with the reviewers if they feel that the functionality was clear from their initial review and from reading #63. I have asked this over in openjournals/joss-reviews#8453

Sounds good to me!

Please merge this PR and I will proceed with the remainder of the editorial checks when I return to work next week.

Will do!