Closed editorialbot closed 3 months ago
editorialbot
commented
1 year ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1109/TPAMI.2022.3217046 is OK
- 10.1109/CVPR.2018.00352 is OK
- 10.1007/s11263-021-01447-x is OK
- 10.21105/joss.04101 is OK
MISSING DOIs
- 10.1109/tpami.2016.2577031 may be a valid DOI for title: Faster R-CNN: Towards Real-Time Object Detection with Region Proposal Networks
- 10.1007/s11263-021-01447-x may be a valid DOI for title: Scale Aware Domain Adaptive Faster R-CNN
- 10.1145/3503161.3548246 may be a valid DOI for title: Improving Transferability for Domain Adaptive Detection Transformers
- 10.1109/wacv51458.2022.00045 may be a valid DOI for title: To miss-attend is to misalign! Residual Self-Attentive Feature Alignment for Adapting Object Detectors
INVALID DOIs
- NoneSoftware report:
github.com/AlDanial/cloc v 1.88 T=12.51 s (4.8 files/s, 385.7 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 29 649 569 2779
YAML 24 75 54 299
Markdown 2 65 0 161
TeX 1 16 0 155
JSON 4 0 0 4
-------------------------------------------------------------------------------
SUM: 60 805 623 3398
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 1082
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
galessiorob
commented
1 year ago 👋 @just-eoghan we're ready to start this review! Mind adding the missing DOIs listed here, please?
🚀 @AnnikaStein, @samirmartins, @ns-rse thank you so much for volunteering to review this paper! Please comment @editorialbot generate my checklist and start your review at your earliest convenience. Let me know if you have any questions.
AnnikaStein
commented
1 year ago 
ns-rse
commented
1 year ago ~@editorialbot start review~
editorialbot
commented
1 year ago I'm sorry @ns-rse, I'm afraid I can't do that. That's something only editors are allowed to do.
ns-rse
commented
1 year ago 
just-eoghan
commented
1 year ago Hey @ns-rse! Agree with all your suggestions so far. Should I make these changes as you suggest them or wait until the review has ended?
ns-rse
commented
1 year ago Hi @just-eoghan,
Sorry for leaving this half-way through, I've tidied up the above checklist, removed the comments and include them below under each section.
As to when to address them I'm not sure, this is my first time reviewing for JOSS. It might perhaps be worth waiting for feedback from other reviewers.
@ns-rse
Yes Source code is available at the URL https://github.com/just-eoghan/Lightning-UDA-detect/
The submitting author has made major contributions to the software, with all git blame checked attributed to
just-eoghan. There are no commits from other authors and I assume the co-authors are supervisors of the work.
NO - It might be an obvious step to experienced users but this package is not available on PyPI nor Conda Forge. This
means that users have to clone the repository from Git and cd into it after creating their Conda environment...
git clone https://github.com/just-eoghan/Lightning-UDA-detect.git
cd Lightning-UDA-detect...only then can they pip install -r requirements.txt. The package would benefit from having the install requirements
documented in pyproject.toml (the current recommended configuration file for installation with
Setuptools framework). This would also allow the
inclusion of metadata about the package, configuration of the black linter which I see is mentioned in some of the
dependencies (and installation of such dependencies can be grouped under project.optional-dependencies), this helps set the stage for publishing the package on PyPI.
Maybe - The claim is that the package simplifies performing certain analyses which otherwise required abandoned packages that have more convoluted installation processes. These have not been attempted but it does appear the process is easier compared to the instructions that have been read although this reviewer was unable to complete running the example.
Its claimed the precision of the models used here exceeds the original but I have not checked this.
Yes authors make it clear that existing tools
(maskrcnn-benchmark) is convoluted and having read the
installation instructions for this tool (which hasn't been updated in 4 years) the presented software
Lightning-UDA-detect does seem considerably to be easier to install and use.
Yes dependencies are detailed in a requirements.txt and installation instructions when followed (subject to the above
comment about needing to git clone the repository) result in successful installation and an environment that allows
the code to run. As noted though, there is no adherence to any of the Python packaging frameworks such as Setuptools,
Poetry or PDM.
Perhaps a more explicit statement of the Python versions supported would be useful (e.g. badges). Installation of dependencies using Python 3.11 failed with
Collecting notebook-shim>=0.2.3 (from nbclassic->jupyterlab==3.6.3->-r requirements.txt (line 28))
Using cached notebook_shim-0.2.3-py3-none-any.whl (13 kB)
INFO: pip is looking at multiple versions of opencv-python-headless to determine which version is compatible with other requirements. This could take a while.
Collecting opencv-python-headless>=4.1.1 (from albumentations==1.3.0->-r requirements.txt (line 30))
Downloading opencv_python_headless-4.7.0.72-cp37-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (49.2 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 49.2/49.2 MB 1.6 MB/s eta 0:00:00
Downloading opencv_python_headless-4.7.0.68-cp37-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (49.2 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 49.2/49.2 MB 763.5 kB/s eta 0:00:00
Downloading opencv_python_headless-4.6.0.66-cp36-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (48.3 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 48.3/48.3 MB 564.1 kB/s eta 0:00:00
Downloading opencv_python_headless-4.5.5.64-cp36-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (47.8 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 47.8/47.8 MB 650.2 kB/s eta 0:00:00
Downloading opencv_python_headless-4.5.5.62-cp36-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (47.7 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 47.7/47.7 MB 856.2 kB/s eta 0:00:00
Downloading opencv-python-headless-4.5.4.60.tar.gz (89.8 MB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 89.8/89.8 MB 553.7 kB/s eta 0:00:00
Installing build dependencies ... error
error: subprocess-exited-with-error
× pip subprocess to install build dependencies did not run successfully.
│ exit code: 1
╰─> [20 lines of output]
Ignoring numpy: markers 'python_version == "3.6" and platform_machine != "aarch64" and platform_machine != "arm64"' don't match your environment
Ignoring numpy: markers 'python_version == "3.7" and platform_machine != "aarch64" and platform_machine != "arm64"' don't match your environment
Ignoring numpy: markers 'python_version == "3.8" and platform_machine != "aarch64" and platform_machine != "arm64"' don't match your environment
Ignoring numpy: markers 'python_version <= "3.9" and sys_platform == "linux" and platform_machine == "aarch64"' don't match your environment
Ignoring numpy: markers 'python_version <= "3.9" and sys_platform == "darwin" and platform_machine == "arm64"' don't match your environment
Ignoring numpy: markers 'python_version == "3.9" and platform_machine != "aarch64" and platform_machine != "arm64"' don't match your environment
Collecting setuptools
Using cached setuptools-68.0.0-py3-none-any.whl (804 kB)
Collecting wheel
Using cached wheel-0.40.0-py3-none-any.whl (64 kB)
Collecting scikit-build
Downloading scikit_build-0.17.6-py3-none-any.whl (84 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 84.3/84.3 kB 566.3 kB/s eta 0:00:00
Collecting cmake
Using cached cmake-3.26.4-py2.py3-none-manylinux2014_x86_64.manylinux_2_17_x86_64.whl (24.0 MB)
Collecting pip
Using cached pip-23.1.2-py3-none-any.whl (2.1 MB)
ERROR: Ignored the following versions that require a different python version: 1.21.2 Requires-Python >=3.7,<3.11; 1.21.3 Requires-Python >=3.7,<3.11; 1.21.4 Requires-Python >=3.7,<3.11; 1.21.5 Requires-Python >=3.7,<3.11; 1.21.6 Requires-Python >=3.7,<3.11
ERROR: Could not find a version that satisfies the requirement numpy==1.21.2 (from versions: 1.3.0, 1.4.1, 1.5.0, 1.5.1, 1.6.0, 1.6.1, 1.6.2, 1.7.0, 1.7.1, 1.7.2, 1.8.0, 1.8.1, 1.8.2, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.10.0.post2, 1.10.1, 1.10.2, 1.10.4, 1.11.0, 1.11.1, 1.11.2, 1.11.3, 1.12.0, 1.12.1, 1.13.0, 1.13.1, 1.13.3, 1.14.0, 1.14.1, 1.14.2, 1.14.3, 1.14.4, 1.14.5, 1.14.6, 1.15.0, 1.15.1, 1.15.2, 1.15.3, 1.15.4, 1.16.0, 1.16.1, 1.16.2, 1.16.3, 1.16.4, 1.16.5, 1.16.6, 1.17.0, 1.17.1, 1.17.2, 1.17.3, 1.17.4, 1.17.5, 1.18.0, 1.18.1, 1.18.2, 1.18.3, 1.18.4, 1.18.5, 1.19.0, 1.19.1, 1.19.2, 1.19.3, 1.19.4, 1.19.5, 1.20.0, 1.20.1, 1.20.2, 1.20.3, 1.21.0, 1.21.1, 1.22.0, 1.22.1, 1.22.2, 1.22.3, 1.22.4, 1.23.0rc1, 1.23.0rc2, 1.23.0rc3, 1.23.0, 1.23.1, 1.23.2, 1.23.3, 1.23.4, 1.23.5, 1.24.0rc1, 1.24.0rc2, 1.24.0, 1.24.1, 1.24.2, 1.24.3, 1.24.4, 1.25.0rc1, 1.25.0)
ERROR: No matching distribution found for numpy==1.21.2
[end of output]
note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error
× pip subprocess to install build dependencies did not run successfully.
│ exit code: 1
╰─> See above for output.
note: This error originates from a subprocess, and is likely not a problem with pip.Could add  to README.md.
Yes - An example is provided using data that is freely available. Having gone through the example the following observations were made.
Weights and Biases
You can automatically log runs to your W&B account by editing configs/logger/wandb
Set entity to the name of your W&B account. Set project to the name of the project you have created in W&B.
This reviewer is not familiar with W&B, but reading the noted config file (which is actually
configs/logger/wandb.yaml) indicates its the site https://wandb.ai better documentation explaining what this is and
how it works would be beneficial.
❱ python run.py experiment=mic_da_cityscapes logger=csv
[2023-07-09 18:16:35,720][torch.distributed.nn.jit.instantiator][INFO] - Created a temporary directory at /tmp/tmpeth8dh8m
[2023-07-09 18:16:35,721][torch.distributed.nn.jit.instantiator][INFO] - Writing /tmp/tmpeth8dh8m/_remote_module_non_scriptable.py
[2023-07-09 18:16:35,789][src.utils.utils][INFO] - Disabling python warnings! <config.ignore_warnings=True>
│ └── _target_: pytorch_lightning.Trainer
├── trainer
[2023-07-09 18:20:52,445][src.train][INFO] - Instantiating trainer <pytorch_lightning.Trainer>
│ min_epochs: 1
│ max_epochs: -1
│ deterministic: false
│ num_sanity_val_steps: 0
│ min_steps: 1
│ devices:
│ accelerator: gpu
self._accelerator_connector = _AcceleratorConnector(
│ - 0
│
├── model
│ └── _target_: src.models.mic_da_frcnn.MicDaFrcnnDetectionModel
│ da_net:
│ weight_decay: 0.0001
lightning_fabric.utilities.exceptions.MisconfigurationException: No supported gpu backend found!
│ _target_: src.models.modules.da_mic.model.MicDaFRCNN
│ backbone_out_channels: 256
│ consit_weight: 0.1
│ img_grl_weight: 0.01
│ ins_grl_weight: 0.1
│ num_classes: 9
│ lr_scheduler: true
│ num_classes: 9
│
├── datamodule
│ └── _target_: src.datamodules.da_cityscapes_datamodule.DaCityscapesDatamodule
│ source_data_dir: /mnt/work/git/hub/Lightning-UDA-detect/data/cityscapes/
│ target_data_dir: /mnt/work/git/hub/Lightning-UDA-detect/data/foggy_cityscapes/
│ train_batch_size: 2
│ pin_memory: false
│ num_workers: 0
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/utilities/argparse.py", line 69, in insert_env_defaults
│ num_classes: 9
│ image_width: 1600
│ image_height: 800
│
├── callbacks
│ └── model_checkpoint:
│ _target_: pytorch_lightning.callbacks.ModelCheckpoint
│ monitor: val/map50
│ mode: max
│ save_top_k: 1
│ save_last: true
│ verbose: false
│ auto_insert_metric_name: false
│ model_summary:
(joss-uda-detect) ❱
│ model_summary:
│ _target_: pytorch_lightning.callbacks.RichModelSummary
│ max_depth: 4
│ rich_progress_bar:
│ _target_: pytorch_lightning.callbacks.RichProgressBar
│
├── logger
│ └── csv:
│ _target_: pytorch_lightning.loggers.csv_logs.CSVLogger
│ save_dir: .
│ name: csv/
│ version: mic_da_cityscapes
│ prefix: ''
│
├── test_after_training
│ └── False
├── seed
│ └── 100101101112115101101107046097105
└── name
└── mic_da_cityscapes
[2023-07-09 18:16:35,848][lightning_fabric.utilities.seed][INFO] - Global seed set to 1292028720
[2023-07-09 18:16:35,850][src.train][INFO] - Instantiating datamodule <src.datamodules.da_cityscapes_datamodule.DaCityscapesDatamodule>
[2023-07-09 18:16:36,750][src.train][INFO] - Instantiating model <src.models.mic_da_frcnn.MicDaFrcnnDetectionModel>
Downloading: "https://download.pytorch.org/models/resnet50-11ad3fa6.pth" to /home/neil/.cache/torch/hub/checkpoints/resnet50-11ad3fa6.pth
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 97.8M/97.8M [04:14<00:00, 403kB/s]
[Masking] Use color augmentation.
[2023-07-09 18:20:52,440][src.train][INFO] - Instantiating callback <pytorch_lightning.callbacks.ModelCheckpoint>
[2023-07-09 18:20:52,442][src.train][INFO] - Instantiating callback <pytorch_lightning.callbacks.RichModelSummary>
[2023-07-09 18:20:52,443][src.train][INFO] - Instantiating callback <pytorch_lightning.callbacks.RichProgressBar>
[2023-07-09 18:20:52,444][src.train][INFO] - Instantiating logger <pytorch_lightning.loggers.csv_logs.CSVLogger>
Traceback (most recent call last):
Error executing job with overrides: ['experiment=mic_da_cityscapes', 'logger=csv']
Traceback (most recent call last):
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/hydra/_internal/instantiate/_instantiate2.py", line 62, in _call_target
return target(*args, **kwargs)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/utilities/argparse.py", line 69, in insert_env_defaults
return fn(self, **kwargs)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/trainer.py", line 393, in __init__
__
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/connectors/accelerator_connector.py", line 154, in __init__
self._accelerator_flag = self._choose_gpu_accelerator_backend()
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/connectors/accelerator_connector.py", line 371, in _choose_gpu_accelerator_backend
raise MisconfigurationException("No supported gpu backend found!")
During handling of the above exception, another exception occurred:
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "run.py", line 30, in main
return train(config)
File "/mnt/work/git/hub/Lightning-UDA-detect/src/train.py", line 61, in train
trainer: Trainer = hydra.utils.instantiate(
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/hydra/_internal/instantiate/_instantiate2.py", line 180, in instantiate
return instantiate_node(config, *args, recursive=_recursive_, convert=_convert_)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/hydra/_internal/instantiate/_instantiate2.py", line 249, in instantiate_node
return _call_target(target, *args, **kwargs)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/hydra/_internal/instantiate/_instantiate2.py", line 64, in _call_target
raise type(e)(
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/hydra/_internal/instantiate/_instantiate2.py", line 62, in _call_target
return target(*args, **kwargs)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/trainer.py", line 393, in __init__
return fn(self, **kwargs)
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/trainer.py", line 393, in __init__
self._accelerator_connector = _AcceleratorConnector(
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/connectors/accelerator_connector.py", line 154, in __init__
self._accelerator_flag = self._choose_gpu_accelerator_backend()
File "/home/neil/.virtualenvs/joss-uda-detect/lib/python3.8/site-packages/pytorch_lightning/trainer/connectors/accelerator_connector.py", line 371, in _choose_gpu_accelerator_backend
raise MisconfigurationException("No supported gpu backend found!")
lightning_fabric.utilities.exceptions.MisconfigurationException: Error instantiating 'pytorch_lightning.trainer.trainer.Trainer' : No supported gpu backend found!
Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace.
...which appears to be because I do not have a dedicated Graphics card to provision GPU on the computer I'm trying
on. It would therefore be useful to state up-front that the package is contingent on having dedicated GPU hardware. I
switched to a laptop with a dedicated GPU with ~4GB RAM, I then encountered memory errors
File "/home/neil/miniconda3/envs/joss-lightning/lib/python3.8/site-packages/torch/autograd/__init__.py", line 200, in backward
Variable._execution_engine.run_backward( # Calls into the C++ engine to run the backward pass
torch.cuda.OutOfMemoryError: CUDA out of memory. Tried to allocate 222.00 MiB (GPU 0; 3.81 GiB total capacity; 2.61 GiB already allocated; 72.69 MiB free; 3.63 GiB reserved in total by PyTorch) If reserved memory is >> allocated memory try setting max_split_size_mb to avoid fragmentation. See documentation for Memory Management and PYTORCH_CUDA_ALLOC_CONF
Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace.I switched a system with higher specification (GM200 TITAN X) and the job ran to completion. It might therefore be prudent to make it clearer up-front what the hardware requirements are.
config/experiments but no guidance is given as to what parameters might be tuned and the implications of doing so. Each file references additional configurations that are located under configs/model/*.yaml and there is no reference to these in the instructions which would likely be prudent.Maybe There is no API as such, rather a set of scripts that run using specified scripts. As mentioned above there is no description of how parameters in the model might be adjusted and the impact this might have on the results, nor how one might go about deriving parameters for new data.
Missing - no automated test-suite is included.
Missing - no guidelines on how to contribute (e.g. CONTRIBUTING.md), how to report issues (e.g. section in
README.md linking to issues), no details of support (e.g. how to contact authors directly).
The summary explains what the package does although does use some technical language, assuming readers understand the difference between labelled and unlabelled data when training algorithms.
Yes the statement is clear and concise making it clear existing tools are cumbersome and the presented software eases the process of undertaking unsupervised image domain specific object detection.
Maybe there is no direct comparison to the existing tools this package references to show how they compare in terms of installation or run-time. There is a comparison of the accuracy of the object detection but this reviewer is unable to verify the metrics quoted for the other software.
Its highlighted that some citations are missing DOIs
galessiorob
commented
1 year ago @ns-rse Thanks for this thorough review! It's so helpful to see the responses to each question. @just-eoghan feel free to start addressing them, and update us here with any changes you have implemented (we'll see them anyways but it's nice to get a ping to the issue.) Thanks!
@AnnikaStein and @samirmartins let me know if you have any questions.
galessiorob
commented
1 year ago 👋 @AnnikaStein @samirmartins checking in, mind updating us on your ETA for the review? Thanks!
samirmartins
commented
1 year ago Hi @galessiorob. As I informed via email, I will be able to provide a precise and comprehensive review at the beginning of August. Therefore, I will need 1 or 2 more weeks to complete this task. Is that okay to you?
Best Regards,
samirmartins
commented
1 year ago galessiorob
commented
1 year ago @samirmartins yes all good, thanks!
samirmartins
commented
1 year ago Dear colleagues (@galessiorob, @just-eoghan),
Here you can find a portion of my review. I will complete it once CityScapes approves my registration request (as seen below, this is an important issue that should be addressed by authors). If you have any questions, feel free to contact me.
Best regards,
I suggest that the authors create a pip installer (PyPI) to make the process more user-friendly. As it stands, everything is "script-based," and the final user has to install some unnecessary libraries (such as black) to run the scripts."
I encountered an error during the installation of requirements.txt on Windows 11 (resolved by installing pycocotools-windows instead of the pycocotools from requirements). How can we resolve this OS-dependent issue? The error description can be found at the end of this message.
To conduct the review, I need to wait for a few days until CityScapes admin approves my request to download the data and test the software. Is it possible to use any open-source data source for testing the tool and include it in the repository? This has, for instance, hindered my ability to test the code while first conducting the article review.
According to the JOSS Reviewer Guidelines, the article should have documentation, tests, continuous integration, and a license. Some highlights: i) I did not find any tests in the repository; ii) The documentation is very concise and should be improved (currently, it is limited to README.md). As a suggestion, consider using mkdocs or a similar tool to build it; iii) the software has a MIT licence and; iv) allows continuous integration.
As pointed out by @ns-rse, some references are missing DOIs.
All commits were made solely by @just-eoghan. Where are the contributions from the other authors of the work?
There are no clear guidelines on how to contribute to the software/report issues/seek support.
--------------------- MENTIONED INSTALLATION ERROR ---------------------
Building wheels for collected packages: pudb, pycocotools, antlr4-python3-runtime, urwid, pathtools, urwid_readline, pyperclip Building wheel for pudb (setup.py) ... done Created wheel for pudb: filename=pudb-2022.1.3-py3-none-any.whl size=70245 sha256=a3f61a016a36c7fa3e7f2f85a62a3011decccee8dd5691493a409c1022089751 Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\5c\38\91\a0a7fa58d205af466e4e732c9d8ca00ee2cdb2e675a03811bc Building wheel for pycocotools (pyproject.toml) ... error error: subprocess-exited-with-error
× Building wheel for pycocotools (pyproject.toml) did not run successfully. │ exit code: 1 ╰─> [17 lines of output] running bdist_wheel running build running build_py creating build creating build\lib.win-amd64-cpython-38 creating build\lib.win-amd64-cpython-38\pycocotools copying pycocotools\coco.py -> build\lib.win-amd64-cpython-38\pycocotools copying pycocotools\cocoeval.py -> build\lib.win-amd64-cpython-38\pycocotools copying pycocotools\mask.py -> build\lib.win-amd64-cpython-38\pycocotools copying pycocotools__init__.py -> build\lib.win-amd64-cpython-38\pycocotools running build_ext Compiling pycocotools/_mask.pyx because it changed. [1/1] Cythonizing pycocotools/_mask.pyx building 'pycocotools.mask' extension C:\Users\Samir\AppData\Local\Temp\pip-build-env-6i4qqvl\overlay\Lib\site-packages\Cython\Compiler\Main.py:381: FutureWarning: Cython directive 'language_level' not set, using '3str' for now (Py3). This has changed from earlier releases! File: C:\Users\Samir\AppData\Local\Temp\pip-install-1in0fdct\pycocotools_66ddde427fb54904a297c7c0f08b89f2\pycocotools_mask.pyx tree = Parsing.p_module(s, pxd, full_module_name) error: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/ [end of output]
note: This error originates from a subprocess, and is likely not a problem with pip. ERROR: Failed building wheel for pycocotools Building wheel for antlr4-python3-runtime (setup.py) ... done Created wheel for antlr4-python3-runtime: filename=antlr4_python3_runtime-4.8-py3-none-any.whl size=141217 sha256=534fb3f8502d16759b177ebaa4c6dfd47ab3eeb17579b68929fa084de75b4fc8 Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\c8\d0\ab\d43c02eaddc5b9004db86950802442ad9a26f279c619e28da0 Building wheel for urwid (setup.py) ... done Created wheel for urwid: filename=urwid-2.1.2-py3-none-any.whl size=235065 sha256=1b4e7ac9864e5bd1fd95aef0f98c733dfb12d8b5d155f54b4716165935401982 Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\28\71\e4\38b5d81438105d0e3db5016cf2eea6fa796d89d96a04451d4d Building wheel for pathtools (setup.py) ... done Created wheel for pathtools: filename=pathtools-0.1.2-py3-none-any.whl size=8801 sha256=49e958d9d60e872e8ff60cfe6f5e5bced79b0f6cce67bf2dc5203b3c2f394208 Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\4c\8e\7e\72fbc243e1aeecae64a96875432e70d4e92f3d2d18123be004 Building wheel for urwid_readline (pyproject.toml) ... done Created wheel for urwid_readline: filename=urwid_readline-0.13-py3-none-any.whl size=7554 sha256=8136581f87d7d5a4ad479e6c0a95c62c2c142e6b92197cebd1e29e1a3c9406cd Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\80\10\cf\57c5430b5e5033995b239456813efa2587b486130a2688ab3d Building wheel for pyperclip (setup.py) ... done Created wheel for pyperclip: filename=pyperclip-1.8.2-py3-none-any.whl size=11137 sha256=37ab5a575353e7b897b4556866d52f97c4002f262633ed3453577e0d37be3fa6 Stored in directory: c:\users\samir\appdata\local\pip\cache\wheels\7f\1a\65\84ff8c386bec21fca6d220ea1f5498a0367883a78dd5ba6122 Successfully built pudb antlr4-python3-runtime urwid pathtools urwid_readline pyperclip Failed to build pycocotools ERROR: Could not build wheels for pycocotools, which is required to install pyproject.toml-based projects
samirmartins
commented
1 year ago Dear (@galessiorob @just-eoghan) ,
So far, I have been unable to download the dataset. They are very large (one is ~10 GB and the other is ~30 GB), and for some reason, the download doesn't complete. I have tried to complete the download from different locations, with different internet providers, but I can't manage to do it.
Therefore, I'm unable to complete the missing part of the review this way. If the authors of the paper have any suggestions regarding this matter, I'm ready to listen to them.
I consider this iteration partially closed, until one of the authors gets back to me regarding this and the other points I've raised above.
Samir
galessiorob
commented
1 year ago @samirmartins Thank you for looking into this downloading issue and for your review!
@AnnikaStein @ns-rse did you run into the same issue? @just-eoghan I'll try this myself, but please advise.
AnnikaStein
commented
1 year ago This is on my agenda, recently came back from vacation. ETA about 1-2 weeks
just-eoghan
commented
1 year ago @samirmartins Thank you for looking into this downloading issue and for your review!
@AnnikaStein @ns-rse did you run into the same issue? @just-eoghan I'll try this myself, but please advise.
Hey, thanks for all your efforts on the review so far!
For the downloading issue, the reason for choosing Cityscapes is because the standard benchmarks for this task are based on that dataset.
This makes it easy to prove that the results from our software, which is much easier to run than the official implementations, correlate with the benchmarks, as can be seen in the provenance logs
The dataset is large and cumbersome, but no standard open-source dataset for benchmarking this task exists.
I didn't run into any issues downloading the dataset, except for creating an account and being approved.
samirmartins
commented
1 year ago Dear colleagues (@galessiorob @just-eoghan),
I managed to download the files and test the code. Both the preprocessing script and the main code work as expected. However, the other questions I raised have not yet been addressed by the authors of the paper.
Sincerely,
Samir Martins
ns-rse
commented
1 year ago Apologies, been on annual leave.
@galessiorob I had some challenges downloading the data at home (by virtue of a slow connection and lack of patience) but was able to download the test data at work in reasonable time (University with decent connection). I had no issues registering to do so.
galessiorob
commented
11 months ago @editorialbot generate pdf
editorialbot
commented
11 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
galessiorob
commented
11 months ago Hi everyone, and apologies for the very delayed response. Circling back on what needs to be done to close this paper:
@just-eoghan from the reviewer's feedback and the items that need to be addressed:
Installation: The paper does mention that "Lightning-UDA-Detect is structured for straightforward installation" but does not provide explicit details on how the installation proceeds. The paper also mentions that integrating with torchvision "reduces the dependencies of Lightning-UDA-Detect and removes the need to compile from source," however, specific installation steps or a link to documentation detailing the installation process are not mentioned. @just-eoghanyou could add a link to your instructions here and a short explanation.
Functionality: The paper confirms the functional claims of the software by comparing the performance of their re-implementations with the officially reported performance of several UDA architectures, it also states that their models achieve or even exceed the official mAP@50 scores. @samirmartins @ns-rse are the review points that allude to functionality satisfied in your opinion with the current version?
Automated tests: Automated tests are not explicitly mentioned, however, the paper a detailed account of the performance evaluation conducted, including mAP@50 validation graphs and maximum mAP@50 scores. The training process and environment (e.g., the type of GPU used, the number of steps, and the seed) are described. If this manual test is meant to replace the expected automated tests, then I suggest adding an explanation as to why.
Community guidelines: The paper mentions that "Contributions to the package are warmly welcome; please open an issue to discuss new features." but the paper does not provide detailed community guidelines nor are they provided in the repo. Please add, here are some examples and tips.
samirmartins
commented
11 months ago Dear @galessiorob,
The points that the authors have not yet considered/answered in my review are:
I suggest that the authors create a pip installer (PyPI) to make the process more user-friendly. As it stands, the final user has to install some unnecessary libraries (such as black) to run the scripts.
I encountered an error during the installation of requirements.txt on Windows 11 (resolved by installing pycocotools-windows instead of the pycocotools from requirements). How can we resolve this OS-dependent issue? The error description can be found at the end of this message.
According to the JOSS Reviewer Guidelines, the article should have documentation, tests, continuous integration, and a license. Some highlights: i) I did not find any tests in the repository; ii) The documentation is very concise and should be improved (currently, it is limited to README.md). As a suggestion, consider using mkdocs or a similar tool to build it.
All commits were made solely by @just-eoghan. Where are the contributions from the other authors of the work?
There are no clear guidelines on how to contribute to the software/report issues/seek support.
galessiorob
commented
11 months ago Thanks for clarifying @samirmartins, @just-eoghan please tend to the issues noted above ane let me know if I can help or clarify anything.
just-eoghan
commented
11 months ago Hi all,
Thank you for your work! Looking forward to getting the last requirements fulfilled to close this paper.
@galessiorob I will remediate the points raised above in relation to installation, functionality, automated tests and community guidelines and revert when complete.
In relation to @samirmartins on Where are the contributions from the other authors these authors are my PhD supervisors and contributed with guidance and mentorship for the project.
galessiorob
commented
11 months ago 👋 @just-eoghan Hi! Checking in to see how the above work is doing.
ns-rse
commented
11 months ago @just-eoghan
Dear @galessiorob,
- I suggest that the authors create a pip installer (PyPI) to make the process more user-friendly. As it stands, the final user has to install some unnecessary libraries (such as black) to run the scripts.
If its of any use I've written about ns-rse - Python Packaging (mirrored on my employers blog).
galessiorob
commented
11 months ago Thanks @ns-rse!
galessiorob
commented
11 months ago @AnnikaStein can you update us on your ETA, please?
galessiorob
commented
10 months ago 👋 @just-eoghan Hi! Checking in again to see how the above work is doing. Let me know if I can help with these last-mile items.
@AnnikaStein can you update us on your ETA, please?
just-eoghan
commented
10 months ago @galessiorob I have made edits for the following points to cover all last-mile items:
Installation Added more detailed install instructions and cover specific OS (Linux, Windows, Mac) Fixed specific error for windows with pycocotools-windows which was raised in the review.
Functionality Proven with provenance data and verified by reviewers
Automated tests
Reasoning for testing is now included in the documentation. A simple test with python run.py experiment=x, easily verifiable through detailed provenance logs, Automation partially blocked by dataset download steps and large file sizes.
Community Guidelines Add contribution guidelines to documentation.
Thanks all for your efforts in facilitating this review!
galessiorob
commented
10 months ago @editorialbot generate pdf
editorialbot
commented
10 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
galessiorob
commented
10 months ago Thanks so much for adding these @just-eoghan !
@samirmartins @ns-rse can you please check the above additions and review the last items on your checklist?
@AnnikaStein given we have not received any updates for your review I'll remove you from this paper so we can go ahead and publish as soon as the other editors' checklists are complete. I hope we can work together in a future paper.
ns-rse
commented
10 months ago Installation instructions, if followed to the letter, are still missing the crucial step of cloning and changing directory into the cloned repository which is required before you can run pip install -r requirements.txt.
Installation: Does installation proceed as outlined in the documentation? NO - It might be an obvious step to experienced users but this package is not available on PyPI nor Conda Forge. This means that users have to clone the repository from Git and cd into it after creating their Conda environment...
git clone https://github.com/just-eoghan/Lightning-UDA-detect.git cd Lightning-UDA-detect...only then can they
pip install -r requirements.txt. The package would benefit from having the install requirements documented in pyproject.toml (the current recommended configuration file for installation with Setuptools framework). This would also allow the inclusion of metadata about the package, configuration of the black linter which I see is mentioned in some of the dependencies (and installation of such dependencies can be grouped underproject.optional-dependencies), this helps set the stage for publishing the package on PyPI.
Also not clear why pycocotools is installed first rather than being listed in requirements.txt (or better as a dependency in pyproject.toml).
The line
All of the required dependencies are stored within the requirements.txt file.
..should probably read...
All of the required dependencies are listed within the
requirements.txtfile.
(although that isn't true of pycocotools).
Can't check this off.
The specific check item is for Automated Tests and there are still no automated tests that run in Continuous Integration (/GitHub Pages) to ensure the functionality of the code. This is a little challenging given the volume of data that needs to be downloaded but could be achieved through mocking of certain objects.
Can't check this off.
The software doesn't have a traditional API as far as I can tell (if it did documentation could be built automatically), rather its a set of scripts that run. The instructions in the README.md explain how to run the model with the test data but lack details of how to go about adapting the code/scripts for any other use case.
Can't check this off.
samirmartins
commented
10 months ago Dear @galessiorob,
The points that the authors have not yet considered/answered in my review are:
* I suggest that the authors create a pip installer (PyPI) to make the process more user-friendly. As it stands, the final user has to install some unnecessary libraries (such as black) to run the scripts. * I encountered an error during the installation of requirements.txt on Windows 11 (resolved by installing pycocotools-windows instead of the pycocotools from requirements). How can we resolve this OS-dependent issue? The error description can be found at the end of this message. * According to the JOSS Reviewer Guidelines, the article should have documentation, tests, continuous integration, and a license. Some highlights: i) I did not find any tests in the repository; ii) The documentation is very concise and should be improved (currently, it is limited to README.md). As a suggestion, consider using mkdocs or a similar tool to build it. * All commits were made solely by @just-eoghan. Where are the contributions from the other authors of the work? * There are no clear guidelines on how to contribute to the software/report issues/seek support.
Dear @galessiorob @just-eoghan ,
Regarding this review, I have the following comments:
"- I suggest that the authors create a pip installer (PyPI) to make the process more user-friendly. As it stands, the final user has to install some unnecessary libraries (such as black) to run the scripts."
Can't check this item, as it has not been addressed yet.
"I encountered an error during the installation of requirements.txt on Windows 11 (resolved by installing pycocotools-windows instead of the pycocotools from requirements). How can we resolve this OS-dependent issue? The error description can be found at the end of this message."
After this iteration, the authors have clarified this in README.md. I'll consider it done.
"According to the JOSS Reviewer Guidelines, the article should have documentation, tests, continuous integration, and a license. Some highlights: i) I did not find any tests in the repository; ii) The documentation is very concise and should be improved (currently, it is limited to README.md). As a suggestion, consider using mkdocs or a similar tool to build it. "
i) I still could not find any tests in the repository (the authors should consider using pytest for this); ii) The documentation is still very concise, limited to README.md. Can't check this item.
"All commits were made solely by @just-eoghan. Where are the contributions from the other authors of the work?"
This was clarified by the authors. Consider it done.
"There are no clear guidelines on how to contribute to the software/report issues/seek support."
They are now present in the current version. Consider it done.
There are also some reference issues, detailed above by @ns-rse. Can't check this item either.
I've updated the checklist, but there are still some issues that need to be solved before publication.
Best regards,
Samir
galessiorob
commented
9 months ago Happy New Year!
Thanks for your comments @samirmartins. @just-eoghan could you please review and address where applicable, thanks!
galessiorob
commented
9 months ago @AnnikaStein mind updating us on the ETA for your review, thanks!
galessiorob
commented
9 months ago 👋 @just-eoghan checking in, when do you expect to be able to address the review comments? Let me know if you have any questions, thanks.
galessiorob
commented
5 months ago 👋 it's been quite some time, @just-eoghan are you still able to continue editing this paper so it can be published?
crvernon
commented
4 months ago :wave: @galessiorob - I think we need to consider pulling this submission if we don't get a response from @just-eoghan within the next week. Have you emailed them as well? If not, could you do so to notify them of this? Thank you!
galessiorob
commented
4 months ago Hey @crvernon, there's no email for @just-eoghan on the JOSS db and I couldn't find it on their Linkedin. How should we proceed?
crvernon
commented
4 months ago @editorialbot remind me in one week
Hi @galessiorob, I found the email address for @just-eoghan in the original submission info. I am sending an email to them and cc'ing you right now. I am going to pause this submission for one week. If we don't here back by then we will unfortunately have to withdraw it. Thanks for checking in!
editorialbot
commented
4 months ago Reminder set for @crvernon in one week
editorialbot
commented
4 months ago :wave: @crvernon, please take a look at the state of the submission (this is an automated reminder).
Submitting author: !--author-handle-->@just-eoghan<!--end-author-handle-- (Eoghan Mulcahy) Repository: https://github.com/just-eoghan/Lightning-UDA-detect Branch with paper.md (empty if default branch): Version: v1.0.0 Editor: !--editor-->@galessiorob<!--end-editor-- Reviewers: @AnnikaStein, @samirmartins, @ns-rse Archive: Pending
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@AnnikaStein & @samirmartins & @ns-rse, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @galessiorob know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @AnnikaStein
📝 Checklist for @ns-rse
📝 Checklist for @samirmartins