[REVIEW]: TUM Open Infra Platform: an open source package for simultaneous viewing and analysis of digital models in the civil engineering domain

whedon commented 3 years ago

Submitting author: !--author-handle-->@pjanck@hugoledoux<!--end-editor-- Reviewers: @aothms, @CBenghi, @abdoulayediak Archive: 10.5281/zenodo.6488493

: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 badge code:

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

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

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

Make sure you're logged in to your GitHub account
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 @hugoledoux 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 @aothms

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@pjanck) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @CBenghi

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@pjanck) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @Abdoulayediak

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@pjanck) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @aothms, @CBenghi 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:

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

watching

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

PDF failed to compile for issue #3061 with the following error:

/app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:in parse': (046f30275eaf56e09414b1ae/Documentation/joss/paper.md): did not find expected comment or line break while scanning a block scalar at line 6 column 1 (Psych::SyntaxError) from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:inparse_stream' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:390:in parse' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:277:inload' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:578:in block in load_file' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:inopen' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:in load_file' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:127:inload_yaml' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:87:in initialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:innew' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:in set_paper' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:58:inprepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in <top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `

'

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=9.26 s (172.3 files/s, 74576.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Expect                          20          18217              0         147227
C++                            533          34614          46249         125209
C/C++ Header                   469          21287          26898          96683
XSD                            105            113           1271          53205
C                               24           2796            664          17791
Qt                              32              0              0          10427
XML                             35             57             14          10223
Bourne Shell                     9            943           1628           9143
Qt Linguist                      3              0              0           8663
WiX source                       2           3392             30           7278
m4                              17           1141             71           7018
SVG                             33             30             30           6275
Python                          37           1997           2307           6137
MSBuild script                  18              0             14           3316
CMake                          107            808           1915           2356
HLSL                            15            536            279           1620
XSLT                             4            153             81           1582
yacc                             1            323             24           1290
Markdown                        18            449              0            996
HTML                             2             94            219            731
DOS Batch                       32             69             63            666
Java                             1            138            293            634
TeX                              4            116              1            478
make                            16            111             47            447
C#                              10             97             43            411
lex                              1             31             23            250
Qt Project                      32             90             16            234
ProGuard                         8             31             30             57
QML                              2             10              4             57
MATLAB                           1              5             13             24
JSON                             1              0              0             21
INI                              2              0              0              9
IDL                              1              0              0              1
-------------------------------------------------------------------------------
SUM:                          1595          87648          82227         520459
-------------------------------------------------------------------------------

Statistical information for the repository '2a2496071f4d3ae8814e6c77' was
gathered on 2021/02/24.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Ana Sanchez                      1             4              0            0.00
Cara Coetzee                    45         75224          95039            4.76
Elvira Khromykh                 11           198            134            0.01
Fabian Schöttl                   7           919            274            0.03
Frédéric Gillioz                 2          2456            523            0.08
Hecht                            1             1              1            0.00
Helge Hecht                    310        367863        1759623           59.47
Jonas Schlenger                 52          2319            881            0.09
Julian Amann                    62       1165069           3021           32.65
Lena Fuchs                      28          4031           2585            0.18
Lucas Duckerschein               2           112             66            0.00
Markus Böckle                   12           460            710            0.03
Mauplatteau                     41          1449            777            0.06
Mauricio Platteau                1           389              0            0.01
Max Werner                       3           100             16            0.00
Rebecca Schill                  31         15013           2536            0.49
Samuils Rulovs                 209          9669           4825            0.41
SamuilsRulovs                    7            10             14            0.00
Stefan Jaud                    429         25919           9279            0.98
Stefan Markic                   77          3003           1489            0.13
ccoetzee                        65          8184           3132            0.32
christophKaiser                 49           882            658            0.04
hechth                          63          4620           4167            0.25

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Ana Sanchez                   4          100.0         16.3                0.00
Cara Coetzee               7568           10.1         19.9               19.24
Elvira Khromykh              71           35.9          1.3               56.34
Fabian Schöttl              704           76.6         40.7               18.89
Frédéric Gillioz           1731           70.5         43.6               18.72
Hecht                         1          100.0         10.6                0.00
Helge Hecht               73881           20.1         28.3               29.61
Jonas Schlenger            1849           79.7          0.9               27.91
Julian Amann             275581           23.7          0.3               18.39
Lena Fuchs                  922           22.9         31.7               37.85
Markus Böckle                76           16.5         20.8               10.53
Mauricio Platteau          1195          307.2          5.3               24.94
Rebecca Schill              144            1.0         17.9               65.28
Samuils Rulovs             6438           66.6          4.1               33.15
Stefan Jaud               23391           90.2          2.7               22.73
ccoetzee                   5412           66.1          5.6               59.15
christophKaiser             519           58.8          7.4               30.06

pjanck commented 3 years ago

@whedon generate pdf from branch joss_submission

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss_submission. Reticulating splines etc...

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:

pjanck commented 3 years ago

@whedon generate pdf from branch joss_submission

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss_submission. Reticulating splines etc...

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:

hugoledoux commented 3 years ago

@whedon add @abdoulayediak as reviewer

whedon commented 3 years ago

OK, @abdoulayediak is now a reviewer

CBenghi commented 3 years ago

Just a quick line to let you know that I'm flat out working until Thursday, but I'll be able to pick up the review swiftly after that. Apologies for the delay.

whedon commented 3 years ago

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

whedon commented 3 years ago

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

pjanck commented 3 years ago

Just wanted to drop by and ask if you need any assistance from our side? How can we help you make some progress?

pjanck commented 3 years ago

@whedon generate pdf from branch joss_submission

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss_submission. Reticulating splines etc...

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:

hugoledoux commented 3 years ago

Just wanted to drop by and ask if you need any assistance from our side? How can we help you make some progress?

@pjanck

At this moment nothing, we expect the first feedback from the reviewers within about 2w usually.

Based on these, you are expected to modify/update your code/docs/paper and discuss with them. Usually it takes a few iterations like this.

aothms commented 3 years ago

Hi @pjanck

Thanks for submitting this. This is obviously very welcomed work by the community. Before I check off the checklist, let me start with some general comments (more might follow, just wanted to get the conversation going).

The paper, nor documentation attempts at contrasting this work with the other libraries for similar purposes. Two of your reviewers work on software libraries with overlapping aims. What are the differences?

License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?

The file is named License.txt I recommend renaming this to LICENSE (I think to have the automated Github license discovery kick in, it isn't recognized now it seems). Which GPL version is used actually?

It is said that the platforms is aimed at providing a development platform, but I find no example usage of how to use the platform as such as a software library (but i know there are some interesting gems in there wrt e.g schema-agnostic template programming)

The paper text mentions a cross platform solution, but only Windows is currently supported. This needs to be better documented and perhaps explained: why only 64bit, why only msvc 2015/17 (for community edition users this is painful as msoft only allows the latest (2019) version to be downloaded for free if you didn't download earlier versions before).

No tags / releases?

Python 2 is really really end of life now (what is Python used for actually as a dependency?)

I find the paper very heavy on self citations

The paper mentions EXPRESS parser, but there is no explanation given what this entails, e.g that it is the modelling langauge used to express IFC, but there is also IfcXML is this supported?

The paper and documentation give very little detail on the implementation strategies (e.g mapping of IFC entity to C++, e.g SELECT to boost::variant / visit_struct) and geometry interpretation (which entities are supported and mapped to what geometric/topological entities).

I think a reflection on usage of Carve as a geometric modelling language would be very appreciated by the community, wrt e.g precision handling, robustness of boolean ops, support for 10303-42 modelling constructs such as surface_curve_swept_area_solid, but perhaps more in favour of Carve things like performance, simplicity and ease of use.

Not all dependencies listed: tumcms/Blue-Framework, zlib, visit_struct, ...

pjanck commented 3 years ago

Hi @aothms

thank you for the comments. Please find our comments below. Additionally, the paper and the branch have been updated accordingly.

The paper, nor documentation attempts at contrasting this work with the other libraries for similar purposes. Two of your reviewers work on software libraries with overlapping aims. What are the differences?

I see that I have skipped over the instruction "including to other software addressing related needs". I added a paragraph to the Statement of Need section. Hopefully, I've cited everything correctly.

The file is named License.txt I recommend renaming this to LICENSE (I think to have the automated Github license discovery kick in, it isn't recognized now it seems). Which GPL version is used actually?

Thanks for the hint, changed accordingly. It is GPL v3.

It is said that the platforms is aimed at providing a development platform, but I find no example usage of how to use the platform as such as a software library (but i know there are some interesting gems in there wrt e.g schema-agnostic template programming)

I'm not sure I understand this comment correctly. Can you please expand on it? Where is that said?

I've added the schema-agnostic template programming - shamelessly stolen. :)

The paper text mentions a cross platform solution, but only Windows is currently supported. This needs to be better documented and perhaps explained: why only 64bit, why only msvc 2015/17 (for community edition users this is painful as msoft only allows the latest (2019) version to be downloaded for free if you didn't download earlier versions before).

We have tried to update to vs2019, see https://github.com/tumcms/Open-Infra-Platform/pull/319 . The clash between the dependencies is stopping the progress, so the update is currently put on ice so that the dependencies clear out their problems. As a consequence we are staying with the versions we are sure that work.

No tags / releases?

Not yet. We will do the first one when we are done with the review so that it can be referenced in the paper.

Python 2 is really really end of life now (what is Python used for actually as a dependency?)

It is needed for a workaround for updating Boost: https://github.com/tumcms/Open-Infra-Platform/blob/development/cmake/UpdateBoostMPL.cmake .

I find the paper very heavy on self citations.

Two reasons:

There aren't many others using the tool. So the "self" is sort of mandatory.
The guidelines provide these instructions:
- "Your paper should not focus on new research results accomplished with the software."
- "Write a short paper in Markdown format using paper.md as file name, including a title, summary, author names, affiliations, and key references [own emphasis]."
- "Mention (if applicable) a representative set of past or ongoing research projects using the software and recent scholarly publications enabled by it."

I'm can remove some, if there are too many. However, with these references, there is no need to lengthen the paper by including their results.

The paper mentions EXPRESS parser, but there is no explanation given what this entails, e.g that it is the modelling langauge used to express IFC, but there is also IfcXML is this supported?

Do you mean the XSD schema? We only support EXPRESS. I added a reference to the standard - ISO 10303-11.

Also, the paper has this sentence explaining what the parser is useful for: "This enables automatic code generation for IFC early-binding library." I've included a sketch from Amann's dissertation that should help with the explanations.

The paper and documentation give very little detail on the implementation strategies (e.g mapping of IFC entity to C++, e.g SELECT to boost::variant / visit_struct) and geometry interpretation (which entities are supported and mapped to what geometric/topological entities).

I believe that that would be too much content for a 250-1000 words paper. I agree that the documentation is scarce - this is one of the reasons we're submitting this paper.

Mapping of SELECTs and the EXPRESS parsing: see the reference [@Hecht:2019:FBI]
Which entities are supported: see https://github.com/tumcms/Open-Infra-Platform/pull/397

I think a reflection on usage of Carve as a geometric modelling language would be very appreciated by the community, wrt e.g precision handling, robustness of boolean ops, support for 10303-42 modelling constructs such as surface_curve_swept_area_solid, but perhaps more in favour of Carve things like performance, simplicity and ease of use.

I've included a mention to Carve in the paper. Or are you having something bigger in mind?

Not all dependencies listed: tumcms/Blue-Framework, zlib, visit_struct, ...

Would you expect a list in the paper? The list can be found at the end of the main Readme. I tried to not repeat information that is available in the repository.

pjanck commented 3 years ago

@whedon generate pdf from branch joss_submission

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss_submission. Reticulating splines etc...

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:

aothms commented 3 years ago

Hi @pjanck thanks for the quick reply

It is said that the platforms is aimed at providing a development platform, but I find no example usage of how to use the platform as such as a software library (but i know there are some interesting gems in there wrt e.g schema-agnostic template programming)

I'm not sure I understand this comment correctly. Can you please expand on it? Where is that said?

I was referring to `Let it be stated here that OIP serves as a prototypical playground for developments' in the article text. If I were to start on a new development (let's say provide an implementation for IfcBlossCurve) where do I begin?

I added a reference to the standard - ISO 10303-11.

11 is the schema language. Do you mean part 21 for the SPF file format (vs part 28 for XML)?

Which entities are supported: see tumcms/Open-Infra-Platform#397

Ok, so that's the unmerged [0] that's obviously vital information. I don't think it's as binary as this, glossing over some of the code I see attributes not being handled e.g [1] and contradicting log messages [2]. But it's a start, let's get this merged and referenced from the docs.

[0] https://github.com/tumcms/Open-Infra-Platform/blob/31a5bd4cd43906e86326b422d1121e7638d60b42/Documentation/markdown/SupportedIFCrepresentations.md [1] https://github.com/tumcms/Open-Infra-Platform/blob/c0f621c18838b249fa6281dbb3cc8b4ffe95dc72/Core/src/IfcGeometryConverter/SolidModelConverter.h#L816 [2] https://github.com/tumcms/Open-Infra-Platform/blob/c0f621c18838b249fa6281dbb3cc8b4ffe95dc72/Core/src/IfcGeometryConverter/SolidModelConverter.h#L823

I've included a mention to Carve in the paper. Or are you having something bigger in mind?

Yes I was hoping for more of a reflection also on the usage of Carve. To me, OpenCascade or CGAL are much more natural choices, but also a lot more complex and probably less performant. This kind of first hand usage experience I think is very valuable.

Would you expect a list in the paper? The list can be found at the end of the main Readme. I tried to not repeat information that is available in the repository.

No readme is fine. I'm not seeing this visit_struct thing in the readme, I didn't do a full side by side check though.

pjanck commented 3 years ago

Hi @aothms , great review!

I was referring to `Let it be stated here that OIP serves as a prototypical playground for developments' in the article text. If I were to start on a new development (let's say provide an implementation for IfcBlossCurve) where do I begin?

Good point. I'm not quite sure how we should/will address this. I believe that a sketch about the program's architecture would be beneficial. Need to take a closer look at this.

11 is the schema language. Do you mean part 21 for the SPF file format (vs part 28 for XML)?

Correct, we parse the schema (specified according to part 11) and generate C++ classes and a reader that reads in part 21 and fills the runtime data. This is also the reason we don't have any IFC classes committed in the repository. See https://github.com/tumcms/Open-Infra-Platform/blob/joss_submission/Documentation/images/express_parser.png (included in the newest version of the paper as Figure 2). We don't support part 28.

Ok, so that's the unmerged [0] that's obviously vital information. I don't think it's as binary as this, glossing over some of the code I see attributes not being handled e.g [1] and contradicting log messages [2]. But it's a start, let's get this merged and referenced from the docs.

About [0]: Yes, it is a WIP. The student has her exams, so we're not pressuring her. Hopefully, with April, she will get to it and we will merge it. About [1] & [2]: Another WIP, another student, similar timeline: https://github.com/tumcms/Open-Infra-Platform/pull/357

It is far from optimal, I agree. But we're on a good track, I believe.

Yes I was hoping for more of a reflection also on the usage of Carve. To me, OpenCascade or CGAL are much more natural choices, but also a lot more complex and probably less performant. This kind of first hand usage experience I think is very valuable.

We'll need to get back to you on this one, I can't remember from the top of my head, why carve was chosen.

No readme is fine. I'm not seeing this visit_struct thing in the readme, I didn't do a full side by side check though.

I see. visit_struct was missing, I've added it only in the joss_submission branch: https://github.com/tumcms/Open-Infra-Platform/tree/joss_submission - this branch will be merged shortly so that the changes will be visible also within the default branch. The branch will still be continued with further improvements as discussed above.

pjanck commented 3 years ago

Hi @aothms - as promised, our answer to the carve question:

Yes I was hoping for more of a reflection also on the usage of Carve. To me, OpenCascade or CGAL are much more natural choices, but also a lot more complex and probably less performant. This kind of first hand usage experience I think is very valuable.

It was a design decision made by IfcPlusPlus creators, whose code we've incorporated into our code and expanded on it. We've also updated the readme on the licenses to better reflect our usage of their code - see https://github.com/tumcms/Open-Infra-Platform/tree/development/Licenses .

aothms commented 3 years ago

Thanks @pjanck I think that kind of proves the point as IfcPlusPlus later also adopted OpenCASCADE https://github.com/ifcquery/ifcplusplus/blob/master/IfcPlusPlus/src/ifcpp/geometry/OCC/GeometryConverterOCC.h not sure how precisely, I think carve is still the main geometry library, but not sure. So it seems like implementations are investigating various geometry libraries. It would be great if first hand experience in using these libraries, their strengths and weaknesses becomes more explicit.

pjanck commented 3 years ago

I agree that a report on the first-hand experience would be beneficial, however: I don't feel we're in a good position to report on the strengths and weaknesses of geometry libraries, since they were not the focus of our developments and we haven't made any comparisons ourselves. We merely used what was available and tweaked it to compile and work with the existing code.

hugoledoux commented 3 years ago

👋 @CBenghi and @abdoulayediak please update us on how your review is going

abdoulayediak commented 3 years ago

Hi all! @hugoledoux here I am finally. First of all, I'd like to apologise for being inactive all this time, as these 2 last months have been very very crazy busy.

@pjanck it is always a pleasure to see some open source contributions, mostly in the field of BIM where there is still a lot to be done. I have been through the repo and the check-list above, and here are my first comments (most of them were already raised by @aothms):

Going through the paper (Statement of need section), the following sentence:

However, they focus mainly on the stable versions of the IFC standard56(like IFC2x3 and IFC4), while OIP focuses primarily on the newer developments.

left me with the impression that OIP does not deal with stable versions of IFC. I had even more that impression while checking the tool itself, as the 'Open' and 'Import' options seem very specific in dealing with specific kind of IFC. I had to try randomly an IFC2x3 file to realise that (as expected), stable release are handled as well. Therefore, I think it would be beneficial to clearly state something here with respect to that. Because if the OIP can handle stable releases just like the other tools do, then it is good plus even. If not, then it should be clearly stated so that the users can limit their expectations.

The self-citation mentioned by @aothms is not that much a problem as you explained. But I think the paper should clarify a bit more explicitly the information that can be found in referred papers without too much details (e.g. mention that details of the OIP components are provided in Hecht & Jaud, 2019).
Another point is the Windows-only support. Although I can see that the "cross-platform" claim was removed from the paper, it would be definitely valuable to state it early somewhere in the ReadMe of the repo.

I still have few other points to check before completing the check-list, but it all sounds on a pretty good track to me.

[For the general discussion] @aothms I found this thread suggesting (among other stuffs that may be of particular interest to you) that OCC is only partially used by IFC++. The latter mostly relies on Carve for its geometric operations.

hugoledoux commented 3 years ago

@pjanck about the self-citations: since JOSS doesn't require new scientific knowledge to be added, but focuses on the implementation of used/known methods, I do not find this a big issue to self-cite 2 papers.

pjanck commented 3 years ago

Hi @abdoulayediak - great to hear from you!

I'm happy that you found some time and first of all: thank you for your comments and your time.

OIP does not deal with stable versions of IFC

This is a true statement for the development branch, which is some ~2 years ahead and currently our main branch (should be set like that in the repo's settings). Previous versiond (I believe even the last installer) could indeed read in stable IFC versions, but could not handle the "old" stuff within the "new" standard development (like an IfcWall in an IFC4x1 file). This is the reason for the major overhaul in the recent three years and our lack of focus for IFC2x3 and IFC4 in the current state. Please observe additionally:

it would be definitely valuable to state [Windows-only support] early somewhere in the ReadMe of the repo.

Added a note to the specialized "setup" readme: https://github.com/tumcms/Open-Infra-Platform/blob/development/Documentation/markdown/SetupHelp.md

I think the paper should clarify a bit more explicitly the information that can be found in referred papers without too much details (e.g. mention that details of the OIP components are provided in Hecht & Jaud, 2019).

I believe that this is enough: "Nowadays, OIP uses the IFC schema and CloudCompare's model as independent internal data models -- explained in detail by @Hecht:2019:FBI." Do you think I should expand (I find the paper already longer as most papers submitted with JOSS).

self-citation

I know that we're quite heavy on self-citations (there are more like a dozen and not only 2 as noted by @hugoledoux ). However, I believe that this is one of the principles of this journal - don't repeat yourself but rather point the readers in the right directions. And I am somehow proud that we've managed to use our software in this many publications :)

I've updated the paper slightly, so I'll be recompiling the pdf again after I press enter.

pjanck commented 3 years ago

@whedon generate pdf from branch joss_submission

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss_submission. Reticulating splines etc...

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:

CBenghi commented 3 years ago

Hi all, I'm ready to resume the review, however it seems that the checkboxes at the top are in read-only mode for me. Is there something I should do to enable them?

hugoledoux commented 3 years ago

@whedon re-invite @CBenghi as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@cbenghi please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

CBenghi commented 3 years ago

Hi @hugoledoux, I clicked on the acceptance link but got an error message and still cannot edit the checkboxes.

But the account is correct. Is there something I can do on my side?

hugoledoux commented 3 years ago

hmmm, can you try another browser or clear cache? Never happened.

If fails I need to ask the person in charge.

CBenghi commented 3 years ago

Hello @hugoledoux I've tried a different browser, private mode and clearing cache with no luck. Not sure what else to do.

hugoledoux commented 3 years ago

@arfon the reviewer cannot accept the invitation, or the invitation is broken. Can you help please?

arfon commented 3 years ago

@whedon re-invite @CBenghi as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@cbenghi please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

arfon commented 3 years ago

@CBenghi - can you try one more time please? I can definitely see an outstanding review for you 👇

https://github.com/openjournals/joss-reviews/invitations

CBenghi commented 3 years ago

Thanks @arfon it worked seamlessly this time around.

CBenghi commented 3 years ago

Hi

A few issues to discuss to improve the paper:

Looking at the authors Contribution and authorship concerns I wonder if you have properly considered the contribution of Cara Coetzee

Author                     Commits    Insertions      Deletions    % of changes

Cara Coetzee                    45         75224          95039            4.76
ccoetzee                        65          8184           3132            0.32

Helge Hecht                    310        367863        1759623           59.47
Jonas Schlenger                 52          2319            881            0.09
Julian Amann                    62       1165069           3021           32.65
Stefan Jaud                    429         25919           9279            0.98

Author                     Rows      Stability          Age       % in comments

Cara Coetzee               7568           10.1         19.9               19.24
ccoetzee                   5412           66.1          5.6               59.15

Helge Hecht               73881           20.1         28.3               29.61
Stefan Jaud               23391           90.2          2.7               22.73
Jonas Schlenger            1849           79.7          0.9               27.91
Julian Amann             275581           23.7          0.3               18.39

On line 17 you mention viewing and analysis, but it's not clear what type of analysis can be performed. I think a more specific set of functional claims would improve the paper.
Perhaps connected to the previous point, on line 57 you mention the specific focus on preliminary Express schemas. This uniqueness could be better highlighted in the project documentation, providing instructions for custom schema implementation.
Mentioning limitations of OIP in comparison to other frameworks could better serve the research community (e.g. what you mentioned about the single IFC version at a time).
Installation documentation could be slightly improved to clarify that plenty of warnings in cmake should be expected.
I've noticed a few apps in the solution that support your testing and QA. I would invite you to produce a quick readme markdown that might help developers understand their purpose and contribute cases.

hugoledoux commented 3 years ago

/ooo May 22 until June 5

hugoledoux commented 3 years ago

@pjanck can you update us on when you think you have time to improve the code so that the review can continue?

If you don't think it's possible in coming weeks, then I could put the review on pause, also a possibility.

openjournals / joss-reviews