editorialbot commented 1 year ago

Submitting author: !--author-handle-->@simonrp84@pdebuyl<!--end-editor-- Reviewers: @arthur-e, @molinav Archive: 10.5281/zenodo.8020079

Status

Status badge code:

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

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

@arthur-e & @molinav, 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:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @pdebuyl 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 @arthur-e

📝 Checklist for @molinav

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 commands

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

@editorialbot generate pdf

editorialbot commented 1 year ago

Software report:

github.com/AlDanial/cloc v 1.88  T=0.08 s (497.6 files/s, 61143.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          16            430            713           1554
reStructuredText                10            265            168            509
Markdown                         2             45              0            131
Jupyter Notebook                 2              0            486            102
YAML                             4              7             11             86
TeX                              1              6              0             63
TOML                             1              4              0             34
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            38            769           1386           2514
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository

editorialbot commented 1 year ago

Wordcount for paper.md is 1090

editorialbot commented 1 year ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.5194/amt-3-813-2010 may be a valid DOI for title: A sea surface reflectance model for (A) ATSR, and application to aerosol retrievals
- 10.5194/amt-5-1889-2012 may be a valid DOI for title: Cloud retrievals from satellite data using optimal estimation: evaluation and application to ATSR
- 10.1364/josa.44.000838 may be a valid DOI for title: Measurement of the roughness of the sea surface from photographs of the sun’s glitter
- 10.1175/bams-d-17-0277.1 may be a valid DOI for title: Pytroll: An open-source, community-driven python framework to process earth observation satellite data
- 10.25080/majora-7b98e3ed-013 may be a valid DOI for title: Dask: Parallel computation with blocked algorithms and task scheduling

INVALID DOIs

- None

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:

pdebuyl commented 1 year ago

Thanks @arthur-e and @molinav for accepting to review! Make sure to issue the command @editorialbot generate my checklist (as a comment in this issue, this will trigger the response of @editorialbot).

Feel free to ask questions directly to the author or to me during the review.

arthur-e commented 1 year ago

Review checklist for @arthur-e

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 https://github.com/simonrp84/PyCoxMunk?
[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 (@simonrp84) 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
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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: Do the authors clearly state 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: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
[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?

My comments, Re: Software paper:

On Line 58 there is reference to "Fig 1c" however, there are only Figures 1a and 1b.
Regarding algorithm details: Given the simplicity (not to mention chicken-and-egg problem) of the chlorophyll content assumption, how does the Cox-Munk algorithm compare to other approaches? Is the primary value of this approach its simplicity? I can imagine the justification is that this technique is "good enough" to tackle the obvious problem of background brightness when trying to estimate the contribution of the atmosphere to TOA reflectance.
No "State of the field," perhaps a few sentences could be added to the "Statement of Need?" Is the Fortran version of ORAC the only comparable library out there? My background is in terrestrial (i.e., non-oceanic) remote sensing, and I wouldn't be surprised if the state of the field was the same: most radiative transfer libraries are in C or Fortran and most are not published as open-source software. So, is the innovation here the Python implementation, the open source, or both and more?

molinav commented 1 year ago

Review checklist for @molinav

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 https://github.com/simonrp84/PyCoxMunk?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
- The license classifier in the setup.cfg is in complete uppercase. I would suggest to write it as in the pyproject.toml file.
- I think that the license metadata entry in pyproject.toml is intended for the license standard name instead of the complete license file. This appears later in the "Meta" block of https://pypi.org/project/pycoxmunk/, which is currently cropped with dots due to excessive string length.
[x] Contribution and authorship: Has the submitting author (@simonrp84) 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
[x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
[x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
[x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
- Tested on Windows: installation with system Python 3.8 and pip, and with miniforge Python 3.9 and conda.
- The library does not contain a __version__ attribute at the package top-level, which might be good to have.

[ ] Functionality: Have the functional claims of the software been confirmed?

I tried to test the functionality of the library by using the Simple_SLSTR.ipynb notebook, but I was not successful. I tried with two Sentinel 3 L1B example files and in both cases it failed when creating the PyCoxMunk instance. Since I am not an expert on Sentinel 3, I can perfectly have done something wrong on my side. Below I list the two granules that I tried to test, the code block that causes the error and the traceback. Former code blocks are executed successfully.

S3A_SL_1_RBT____20230203T041025_20230203T041325_20230204T123946_0179_095_104_3060_PS1_O_NT_004.SEN3
S3B_SL_1_RBT____20230204T111200_20230204T111500_20230204T131718_0179_075_365_1980_PS2_O_NR_004.SEN3

# Create the PyCoxMunk class for processing
pcm = PyCoxMunk(scn, bnames)


---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File c:\ProgramData\pyenv\pyenv-win\versions\py39\lib\site-packages\satpy\utils.py:346, in get_satpos(data_arr, preference, use_tle)
345 try:
--> 346     lon, lat = _get_sat_lonlat(data_arr, lonlat_prefixes)
347     alt = _get_sat_altitude(data_arr, alt_prefixes)

File c:\ProgramData\pyenv\pyenv-win\versions\py39\lib\site-packages\satpy\utils.py:381, in _get_sat_lonlat(data_arr, key_prefixes) 380 def _get_sat_lonlat(data_arr, key_prefixes): --> 381 orb_params = data_arr.attrs["orbital_parameters"] 382 lon_keys = [prefix + "longitude" for prefix in key_prefixes]

KeyError: 'orbital_parameters'

During handling of the above exception, another exception occurred:

KeyError Traceback (most recent call last) File c:\ProgramData\pyenv\pyenv-win\versions\py39\lib\site-packages\pycoxmunk\CM_SceneGeom.py:39, in cm_calcangles(inscn, refband) 38 suna, sunz = _get_sun_angles(inscn[refband]) ---> 39 sata, satz = _get_sensor_angles(inscn[refband]) 41 inscn['satellite_azimuth_angle'] = inscn[refband].copy()

File c:\ProgramData\pyenv\pyenv-win\versions\py39\lib\site-packages\satpy\modifiers\angles.py:445, in _get_sensor_angles(data_arr) 444 preference = satpy.config.get('sensor_angles_position_preference', 'actual') --> 445 sat_lon, sat_lat, sat_alt = get_satpos(data_arr, preference=preference) ... ---> 51 raise KeyError("Input scene does not contain reference dataset, please check inputs.") 52 except ValueError: 53 raise ValueError("Cannot retrieve solar and satellite angles, please compute manually and specify.")

KeyError: 'Input scene does not contain reference dataset, please check inputs.'

- [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**: Do the authors clearly state 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.
- [ ] **Example usage:** Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
   - *The library provides examples of usage in form of Jupyter Notebooks. The examples rely on external files that should be present in the user system in advance. The download of some files requires the registration on an external site. I am not sure if this could be a problem, i.e. if examples should be self-contained.*
- [x] **Functionality documentation:** Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
   - *The library is documented completely, in the source code and also with possibility to build docs with Sphinx.*
   - *I could not find the generated docs uploaded to a public location. Are the docs available without building from source? I think this would be a good idea.*
   - *As a suggestion for future releases, I realised that most of the internals (functions and their arguments) are described "handwritten" in the rst files. This may cause you a problem of maintenance in the future if there are any kind of changes in the source code (rst docs and source code need to be synchronised regularly). Sphinx allows you to pick the content of your docstrings to build the low-API documentation automatically. You may save time in the future by keeping all these descriptions in the source code docstrings and then build from them, and keep the "handwritten" rst files only for the general overview of the library and the extended descriptions of algorithms.*
   - *Minor corrections may be performed based on the results of running `make html` on Debian, see warnings/errors below:*

vic@onyx:/mnt/c/Users/vic/Desktop/scratch/PyCoxMunk/docs$ make html Running Sphinx v6.1.3 making output directory... done building [mo]: targets for 0 po files that are out of date writing output... building [html]: targets for 10 source files that are out of date updating environment: [new config] 10 added, 0 changed, 0 removed reading sources... [100%] quickstart /mnt/c/Users/vic/Desktop/scratch/PyCoxMunk/docs/api_cmwind.rst:4: WARNING: Title underline too short.

The CM_Shared_Wind Module

/mnt/c/Users/vic/Desktop/scratch/PyCoxMunk/docs/api_cmwind.rst:83: ERROR: Unexpected indentation. /mnt/c/Users/vic/Desktop/scratch/PyCoxMunk/docs/quickstart.rst:110: WARNING: Explicit markup ends without a blank line; unexpected unindent. looking for now-outdated files... none found pickling environment... done checking consistency... done preparing documents... done writing output... [100%] quickstart generating indices... genindex done writing additional pages... search done copying images... [100%] _img/CM_Overlay.jpg copying static files... done copying extra files... done dumping search index in English (code: en)... done dumping object inventory... done build succeeded, 3 warnings.

The HTML pages are in _build/html.

- [x] **Automated tests:** Are there automated tests or manual steps described so that the functionality of the software can be verified?
   - *The library contains a complete test suite that can be used with e.g. `pytest`. The coverage is essentially 100% (only one line is missing in coverage). When running the tests, I observed that a significant number of warnings is issued. Most of them seem to be expected warnings by `pycoxmunk`, so a future version could also include assertions related to these warnings in the tests. Other warnings are related to uses of deprecated members from the dependencies: these warnings are more important to solve though because in the future they may cause an error in `pycoxmunk`. See logs below:*

============================= test session starts ============================= platform win32 -- Python 3.9.13, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 rootdir: C:\Users\vic\Desktop\scratch\PyCoxMunk plugins: cov-3.0.0 collected 32 items

pycoxmunk\Tests\test_CMCalcs.py ......... [ 28%] pycoxmunk\Tests\test_CM_main.py ......... [ 56%] pycoxmunk\Tests\test_PixMask.py .. [ 62%] pycoxmunk\Tests\test_SceneGeom.py .... [ 75%] pycoxmunk\Tests\test_SharedWind.py . [ 78%] pycoxmunk\Tests\test_Utils.py ... [ 87%] pycoxmunk\Tests\test_WaterConsts.py .... [100%]

============================== warnings summary =============================== pycoxmunk/Tests/test_CMCalcs.py: 61 warnings pycoxmunk/Tests/test_CM_main.py: 83 warnings pycoxmunk/Tests/test_PixMask.py: 4 warnings pycoxmunk/Tests/test_SceneGeom.py: 31 warnings pycoxmunk/Tests/test_SharedWind.py: 9 warnings pycoxmunk/Tests/test_Utils.py: 5 warnings C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_Utils.py:125: DeprecationWarning: np.float is a deprecated alias for the builtin float. To silence this warning, use float by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use np.float64 here. Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations if type(in_val) == float or type(in_val) == np.float64 or type(in_val) == np.float:

pycoxmunk/Tests/test_CMCalcs.py: 9 warnings pycoxmunk/Tests/test_CM_main.py: 9 warnings pycoxmunk/Tests/test_SceneGeom.py: 6 warnings pycoxmunk/Tests/test_SharedWind.py: 1 warning C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_SceneGeom.py:192: UserWarning: Some solar zenith values out of range. Clipping. warnings.warn("Some solar zenith values out of range. Clipping.")

pycoxmunk/Tests/test_CMCalcs.py: 9 warnings pycoxmunk/Tests/test_SceneGeom.py: 5 warnings pycoxmunk/Tests/test_SharedWind.py: 1 warning C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_SceneGeom.py:206: UserWarning: Some satellite zenith values out of range. Clipping. warnings.warn("Some satellite zenith values out of range. Clipping.")

pycoxmunk/Tests/test_CMCalcs.py::TestCMCalcs::test_compute_bands C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_Calcs.py:64: UserWarning: Warning: Band wavelength 0.22 is less than PyCoxMunk minimum wavelength 0.47 warnings.warn(f"Warning: Band wavelength {cwvl} is less than PyCoxMunk minimum wavelength {wvl_keys[0]}")

pycoxmunk/Tests/test_CMCalcs.py::TestCMCalcs::test_compute_bands C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_Calcs.py:64: UserWarning: Warning: Band wavelength 0.47 is less than PyCoxMunk minimum wavelength 0.47 warnings.warn(f"Warning: Band wavelength {cwvl} is less than PyCoxMunk minimum wavelength {wvl_keys[0]}")

pycoxmunk/Tests/test_CMCalcs.py::TestCMCalcs::test_compute_bands C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_Calcs.py:67: UserWarning: Warning: Band wavelength 4.1 is greater than PyCoxMunk maximum wavelength 3.7 warnings.warn(f"Warning: Band wavelength {cwvl} is greater than PyCoxMunk maximum wavelength {wvl_keys[-1]}")

pycoxmunk/Tests/test_CM_main.py: 12 warnings C:\ProgramData\pyenv\pyenv-win\versions\py39\lib\site-packages\pyresample\geometry.py:2472: DeprecationWarning: 'get_lonlats_dask' is deprecated, please use 'get_lonlats' with the 'chunks' keyword argument specified. warnings.warn("'get_lonlats_dask' is deprecated, please use "

pycoxmunk/Tests/test_CM_main.py: 12 warnings C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_Shared_Wind.py:69: RuntimeWarning: invalid value encountered in double_scalars self.wd = da.arccos(self.v10 / self.ws)

pycoxmunk/Tests/test_SceneGeom.py::TestSceneGeom::test_bounds_checker pycoxmunk/Tests/test_SceneGeom.py::TestSceneGeom::test_bounds_checker C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_SceneGeom.py:220: UserWarning: Some solar azimuth values out of range. Scaling. warnings.warn("Some solar azimuth values out of range. Scaling.")

pycoxmunk/Tests/test_SceneGeom.py::TestSceneGeom::test_bounds_checker pycoxmunk/Tests/test_SceneGeom.py::TestSceneGeom::test_bounds_checker pycoxmunk/Tests/test_SceneGeom.py::TestSceneGeom::test_shapechecker C:\Users\vic\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_SceneGeom.py:234: UserWarning: Some satellite azimuth values out of range. Scaling. warnings.warn("Some satellite azimuth values out of range. Scaling.")

-- Docs: https://docs.pytest.org/en/stable/warnings.html ====================== 32 passed, 265 warnings in 12.80s ======================


- [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
   - *Included as part of the main README.*

### 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:** Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
- [ ] **State of the field:** Do the authors describe how this software compares to other commonly-used packages?
   - *There is no reference to other pieces of software apart from the Fortran implementation `pycoxmunk` is based on. In general, different groups may have their own Cox-Munk implementations for internal purposes but are not available to the public. Currently I can only think of the Cox-Munk code that comes as a supplement of VLIDORT, but I am not sure if this may be considered as a library accessible to the general public.*
- [x] **Quality of writing:** Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
   - *The paper is well written. Below I just add some minor suggestions:*
      - *Line 18: here "chlorophyll" might go in lowercase.*
      - *Line 49: "there's" into "there is".*
      - *Lines 51 and 58: I am not sure if the figure references are correct, they might need to be Fig. 1a and Fig. 1b instead of Fig. 1b and Fig. 1c, respectively.*
- [ ] **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](https://pandoc.org/MANUAL.html#extension-citations)?
   - *The references do not seem to render in a consistent manner. They start with (author, year), but later we may find [Poulsen et al., 2012; orac:2022] or [@raspaud:2018]. This needs to be reviewed before the final document is published.*

molinav commented 1 year ago

@simonrp84 My review is ready. You may find my comments in italics after each of the tick boxes. I also want to apologise for the delay in the review, I did not have enough time to look thoroughly until this weekend.

simonrp84 commented 1 year ago

Thanks for the reviews! I've added tests for the UserWarnings and have removed the numpy related ones DeprecationWarnings. Can't do much about the numexpr warning as that comes from an upstream package, but I guess they'll fix it before python 3.12 arrives.

Regarding the examples, the SLSTR example is indeed troublesome - I think something has changed in the satpy library that's causing this but I can't pin it down. Will investigate further and I'll also add an example that uses truly free and open data (probably NOAA's GOES-16) so that you don't need to register to download the example data. I had also thought of hosting the data myself, but that could cause problems in the future so is best avoided, IMO.

simonrp84 commented 1 year ago

@molinav @arthur-e Thank you for your review comments - I appreciate them.

I have now pushed changes to both the main branch and the paper branch that should address the issues you raise. On main:

Added some tests to catch the UserWarnings about thresholds being exceeded. These warnings are expected, so good to catch them.
Have tried to fix as many DeprecationWarnings as possible. I think the only one remaining is from an upstream source that I can't fix.
Fixed the SLSTR example notebook, it didn't work due to a silly error on my part.
Added a new GOES/ABI example notebook. It has similar functionality to the SLSTR notebook but does not require the user to register, it uses freely available data from NOAA's big data project that's downloaded directly from the example.
Fixed the license to be consistent, so hopefully it'll show up correctly on pypi now.
Updated the doc files to remove warnings during built. @molinav Thanks for your thoughts on automating the docs, this isn't something I've done before but I'll look into how to do it for the future - sounds useful! Regarding the docs being public: They should be available here: https://pycoxmunk.readthedocs.io/en/latest/, which is linked to from the main git page for the repo.

On paper:

Fixed the references, they should display correctly now.
Refactored the paper somewhat, so there's now statement of need and a statement of field sections. Hopefully this satisfies your feedback
Corrected the typos.

Please let me know what your opinions are on this updated version.

simonrp84 commented 1 year ago

@editorialbot generate pdf

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:

arthur-e commented 1 year ago

@simonrp84 Although I still can't get a good result from the SLSTR example, I was able to run the GOES-ABI example and get a nice, specular sun spot image, along with three ABI band images that look good.

I think the last change I would request would be to document the test suite. There are many options for testing in Python, so even though it is very easy to run the suite with pytest, it would be good to add this directive to the README and/or the Documentation.

molinav commented 1 year ago

Hi, @simonrp84! I re-checked everything after re-building the library from the latest main branch. I come with a second round of comments:

About the manuscript: just two minor text edits.
- [ ] Line 78: no comma required between "main class" and "PyCoxMunk" (or surround "PyCoxMunk" with two commas).
- [ ] Line 80: heigh => height (or high? I am not a native speaker).
About the GitHub workflow:
- For a pure Python library like pycoxmunk it is not something dramatic, but it is good practice to run the test suite against the installed library instead of the cloned repository folder. This way, you ensure that the generated sdists and wheels are consistent and not broken. From former experiences (making this mistake myself), your library could depend on auxiliary data files that are not packed into the sdist/wheel properly, and your tests would pass because this data is in the cloned repo, but the distributable package would be broken because of not having this data. The workflow update should contain something like below before pytest (if using the new build mechanism, as there is no legacy setup file):
```
python -m pip install build
python -m build
python -m pip install dist/*.whl
```
About the docs:
- Just for reference, continuing about the automation of docs in the future, I can recommend that you look at how big scientific libraries are doing, the docstring syntax is easy to follow (see e.g. the numpy.take function docstring).
About the license:
- [ ] The license metadata needs one additional correction. It is still saying license = { file = "GPLv3" }, but after the first correction this attribute does not refer to a file anymore. Therefore it should say license = { text = "GPLv3" } (according to the example in the setuptools docs).
- [ ] From what I see in the top-level comment of each module, it looks like you would allow to use the library also with any version after GPLv3. However, I think that the GPLv3 classifier refers to the GPL-3.0-only variant (note that there is a License :: OSI Approved :: GNU General Public License v3 or later (GPLv3+) classifier, see here). Due to the special nature of the GPL-like licenses, I would be cautious about setting this attribute consistently everywhere. I had some issues in the past, e.g. trying to resolve the license implications about GEOS because the license and library headers say one thing (ambiguity if it is LGPLv2.1 or LGPLv2.1+), but the main author says a different thing (LGPLv2.1 only).
About the tests:
- [ ] The pyproject.toml now has an update to ignore RuntimeWarning when running pytest. Using "ignore::RuntimeWarning" is too broad and may hide you unexpected warnings in the future. If you are trying to skip the warning coming from da.arccos, I would replace the filter to "ignore::RuntimeWarning:pycoxmunk.CM_Shared_Wind:" to narrow down a bit. Still, I would check if this RuntimeWarning is coming from passing something to da.arccos outside [-1, 1], because in that case the warning is expected and should be caught as part of the running test suite. Or, if you want to gracefully return nans without warnings under these circumstances, the warning filtering should be performed inside CMSharedWind.calc_wind.
- [ ] I need to check it deeper, but on Windows I am getting errors when running pytest due to warnings not being thrown. Tested with Python 3.9 and Python 3.10, see the tracebacks below. As said, I will review this a bit more on my side, because your GitHub workflow is running on Ubuntu with Python 3.10 and the tests are passing, so it might be an OS-specific issue or I might have done something wrong when preparing the virtual environments. The tracebacks below correspond to the current main branch, if I move to the v1.0.0 tag then the tests are passing (with the warnings mentioned in my first review), so something in between may be causing this change.
```
(joss39) [vic@surface] C:\Users\vic\Desktop\scratch\PyCoxMunk> python -m pytest --cov=pycoxmunk --cov-report html pycoxmunk\Tests
================================================= test session starts ==================================================
platform win32 -- Python 3.9.13, pytest-7.2.1, pluggy-1.0.0
rootdir: C:\Users\vic\Desktop\scratch\PyCoxMunk, configfile: pyproject.toml
plugins: cov-4.0.0
collected 32 items
```

pycoxmunk\Tests\test_CMCalcs.py ......... [ 28%] pycoxmunk\Tests\test_CM_main.py .....FFFF [ 56%] pycoxmunk\Tests\test_PixMask.py .. [ 62%] pycoxmunk\Tests\test_SceneGeom.py .... [ 75%] pycoxmunk\Tests\test_SharedWind.py . [ 78%] pycoxmunk\Tests\test_Utils.py ... [ 87%] pycoxmunk\Tests\test_WaterConsts.py .... [100%]

======================================================= FAILURES ======================================================= __ TestCMMain.test_wind_setup __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000179D68EF8E0>

def test_wind_setup(self):
    """Test that winds are set up correctly."""
    mocker = mock.MagicMock()
    mocker.return_value = True
    with mock.patch('pycoxmunk.CM_Main.CMSharedWind', mocker):
        with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):

          pcm = PyCoxMunk(self.scn_good, self.good_bnd_names)
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [].

pycoxmunk\Tests\test_CM_main.py:168: Failed _ TestCMMain.test_pixmask __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000179D68EFA30>

def test_pixmask(self):
    """Test that pixmask is correctly initialised in the class."""
    with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):

      pcm = PyCoxMunk(self.scn_good, self.good_bnd_names)
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk\Tests\test_CM_main.py:193: Failed ___ TestCMMain.test_retr_cm ____

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000179D68EF9D0> mock_cmr_func =

@mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
def test_retr_cm(self, mock_cmr_func):
    """Tests for the main routine that retrieves reflectance."""

    cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
    cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

    # This section tests that cox_munk reflectance is computed, but not BRDF
    # Recreate scene in case previous tests have messed with it.
    self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
    mock_cmr_func.return_value = self._make_mocked_cm_refl()
    with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):

      pcm = PyCoxMunk(self.scn_good, self.good_bnd_names, mask_bad=False, do_brdf=False)
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk\Tests\test_CM_main.py:210: Failed _ TestCMMain.test_deleter __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000179D68EFB20> mock_cmr_func =

@mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
def test_deleter(self, mock_cmr_func):
    """Test that deletions are done correctly."""

    cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
    cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

    tmp_arr = np.array([1, 4., 64])

---------- coverage: platform win32, python 3.9.13-final-0 ----------- Coverage HTML written to dir htmlcov

=============================================== short test summary info ================================================ FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_wind_setup - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_pixmask - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_retr_cm - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_deleter - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.

(joss310) [vic@surface] C:\Users\vic\Desktop\scratch\PyCoxMunk> python -m pytest --cov=pycoxmunk --cov-report html pycoxmunk\Tests ================================================= test session starts ================================================== platform win32 -- Python 3.10.10, pytest-7.2.1, pluggy-1.0.0 rootdir: C:\Users\vic\Desktop\scratch\PyCoxMunk, configfile: pyproject.toml plugins: cov-4.0.0 collected 32 items

pycoxmunk\Tests\test_CMCalcs.py ......... [ 28%] pycoxmunk\Tests\test_CM_main.py .....FFFF [ 56%] pycoxmunk\Tests\test_PixMask.py .. [ 62%] pycoxmunk\Tests\test_SceneGeom.py .... [ 75%] pycoxmunk\Tests\test_SharedWind.py . [ 78%] pycoxmunk\Tests\test_Utils.py ... [ 87%] pycoxmunk\Tests\test_WaterConsts.py .... [100%]

======================================================= FAILURES ======================================================= __ TestCMMain.test_wind_setup __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x000001CDF7EBE200>

def test_wind_setup(self):
    """Test that winds are set up correctly."""
    mocker = mock.MagicMock()
    mocker.return_value = True
    with mock.patch('pycoxmunk.CM_Main.CMSharedWind', mocker):

      with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [].

pycoxmunk\Tests\test_CM_main.py:167: Failed _ TestCMMain.test_pixmask __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x000001CDF7EBDDE0>

def test_pixmask(self):
    """Test that pixmask is correctly initialised in the class."""

  with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk\Tests\test_CM_main.py:192: Failed ___ TestCMMain.test_retr_cm ____

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x000001CDF7EBDEA0> mock_cmr_func =

@mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
def test_retr_cm(self, mock_cmr_func):
    """Tests for the main routine that retrieves reflectance."""

    cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
    cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

    # This section tests that cox_munk reflectance is computed, but not BRDF
    # Recreate scene in case previous tests have messed with it.
    self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
    mock_cmr_func.return_value = self._make_mocked_cm_refl()

  with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk\Tests\test_CM_main.py:209: Failed _ TestCMMain.test_deleter __

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x000001CDF7EBDB40> mock_cmr_func =

@mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
def test_deleter(self, mock_cmr_func):
    """Test that deletions are done correctly."""

    cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
    cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

    tmp_arr = np.array([1, 4., 64])

    # Test for case where we don't delete intermediate data
    self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
    mock_cmr_func.return_value = self._make_mocked_cm_refl()

  with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. E The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk\Tests\test_CM_main.py:268: Failed

---------- coverage: platform win32, python 3.10.10-final-0 ---------- Coverage HTML written to dir htmlcov

=============================================== short test summary info ================================================ FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_wind_setup - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_pixmask - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_retr_cm - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_deleter - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. ============================================ 4 failed, 28 passed in 50.05s =============================================


- About `Simple_GOES_ABI.ipynb`: thanks for adding this example, I was able to run it without issues. I have two suggestions:

  - [ ] "An library is required for this notebook [...]" => "The following libraries are required for this notebook: `h5netcdf`, `rasterio` and `s3fs`". The first two libraries were not mentioned and I only realised when getting `ImportError` at runtime. 
  - [ ] "We do not apply and cloud or land masking" => "We do not apply any cloud or land masking".

- About `Simple_SLSTR.ipynb`:

  - [ ] For me, this Jupyter notebook still does not work. The error occurs at the same cell as in the first round. I also realised that this notebook has a hidden dependency. Is it expected to have `netCDF4`, `h5netcdf`, or any of them?
```python
# Create the PyCoxMunk class for processing
pcm = PyCoxMunk(scn, bnames)

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File c:\Users\vic\Desktop\scratch\joss310\lib\site-packages\satpy\utils.py:345, in get_satpos(data_arr, preference, use_tle)
    344 try:
--> 345     lon, lat = _get_sat_lonlat(data_arr, lonlat_prefixes)
    346     alt = _get_sat_altitude(data_arr, alt_prefixes)

File c:\Users\vic\Desktop\scratch\joss310\lib\site-packages\satpy\utils.py:380, in _get_sat_lonlat(data_arr, key_prefixes)
    379 def _get_sat_lonlat(data_arr, key_prefixes):
--> 380     orb_params = data_arr.attrs["orbital_parameters"]
    381     lon_keys = [prefix + "longitude" for prefix in key_prefixes]

KeyError: 'orbital_parameters'

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
File ~\Desktop\scratch\PyCoxMunk\pycoxmunk\CM_SceneGeom.py:39, in cm_calcangles(inscn, refband)
     38 suna, sunz = _get_sun_angles(inscn[refband])
---> 39 sata, satz = _get_sensor_angles(inscn[refband])
     41 inscn['satellite_azimuth_angle'] = inscn[refband].copy()

File c:\Users\vic\Desktop\scratch\joss310\lib\site-packages\satpy\modifiers\angles.py:447, in _get_sensor_angles(data_arr)
    446 preference = satpy.config.get('sensor_angles_position_preference', 'actual')
--> 447 sat_lon, sat_lat, sat_alt = get_satpos(data_arr, preference=preference)
...
---> 51     raise KeyError("Input scene does not contain reference dataset, please check inputs.")
     52 except ValueError:
     53     raise ValueError("Cannot retrieve solar and satellite angles, please compute manually and specify.")

KeyError: 'Input scene does not contain reference dataset, please check inputs.'

pdebuyl commented 1 year ago

Hi all, sorry for not checking on the review .Thanks @arthur-e and @molinav for the review so far!

@simonrp84 I also had trouble with the SLSTR example. Even though the data requires registration, could you provide a "ready to type" query to fetch the data? We don't have those via EUMETCast at my institution, so I used https://scihub.copernicus.eu and picked a region at random and SLSTR as the product. I'll check again when at work though since you update the code.

simonrp84 commented 1 year ago

@pdebuyl I have now added another example, using GOES/ABI data, that should work out-of-the-box without the user having to download any files. It's all done within the notebook itself.

After some time having to work on other things I'm now able to look at PyCoxMunk again and hope to address all the review comments in the next day or two.

simonrp84 commented 1 year ago

@arthur-e Thanks for the suggestion regarding some documentation for the tests. I've now added a brief section to the readme and a page to readthedocs that describes how to run / use the tests.

simonrp84 commented 1 year ago

@molinav Thanks for your additional comments.

paper: I hadn't spotted the two typos you mention in the paper - have corrected those now!

workflow: Interesting, I've added that to the workflow file and tests are passing, so hopefully that's been a simple change that could help in the future.
docs: Thanks for the suggestion. I'll definitely bear that in mind for future projects; seems like a timesaver.
license: Yes, you're right. I've now corrected those inconsistencies across the various files to that v3 only is specifies and have also changed file to text.
RuntimeWarning : I have removed this code from the pyproject.toml again - I think I added that for testing and forgot to remove it. The tests aren't producing a RuntimeWarning now.
Windows test errors: I can't reproduce this, unfortunately. I did all the development on windows and didn't notice any errors like this, and have also just tried to reproduce on a fresh windows VM with no success, the tests run OK. Can you confirm if you still see these errors?
Dependencies for Simple_GOES_ABI.ipynb: You're right, I thought these were dependencies of satpy and hence installed as pycoxmunk requires satpy. But they're optional rather than required dependencies in satpy. Have now added these to the notebook.
Problems with Simple_SLSTR.ipynb: Just to check, are you trying to run the latest version from github main branch or the one from the conda / pypi release? I've not made a new release yet so the latter ones will still be broken. For me, the notebook on github works OK on a clean conda environment for both windows and linux. (steps I took are listed here: https://github.com/simonrp84/PyCoxMunk/issues/5 )

As long as you have the notebook from github, there's no need to install pycoxmunk from github. The version on conda works fine. Please let me know if you still experience problems with the notebook and I'll dig deeper.

pdebuyl commented 1 year ago

For info if someone else is interested:

import cdsapi

c = cdsapi.Client()

c.retrieve(
    'reanalysis-era5-single-levels',
    {
        'product_type': 'reanalysis',
        'variable': [
            '10m_u_component_of_wind', '10m_v_component_of_wind',
        ],
        'year': '2016',
        'month': '03',
        'day': '26',
        'time': '15:00',
        'format': 'netcdf',
    },
    'download.nc')

will provide the ERA5 netcdf data. (requires an account with the cds and pip install cdsapi)

pdebuyl commented 1 year ago

================================================================================ test session starts =================================================================================                                                  
platform linux -- Python 3.10.9, pytest-7.2.2, pluggy-1.0.0                                                                                                                                                                             
rootdir: /tmp/PyCoxMunk                                                                                                                                                                                                                 
plugins: anyio-3.6.2                                                                                                                                                                                                                    
collected 32 items                                                                                                                                                                                                                      

pycoxmunk/Tests/test_CMCalcs.py .........                                                                                                                                      [ 28%]                                                   
pycoxmunk/Tests/test_CM_main.py .....FFFF                                                                                                                                      [ 56%]                                                   
pycoxmunk/Tests/test_PixMask.py ..                                                                                                                                             [ 62%]                                                   
pycoxmunk/Tests/test_SceneGeom.py ....                                                                                                                                         [ 75%]                                                   
pycoxmunk/Tests/test_SharedWind.py .                                                                                                                                           [ 78%]                                                   
pycoxmunk/Tests/test_Utils.py ...                                                                                                                                              [ 87%]                                                   
pycoxmunk/Tests/test_WaterConsts.py ....                                                                                                                                       [100%]                                                   

====================================================================================== FAILURES ======================================================================================                                                  
_____________________________________________________________________________ TestCMMain.test_wind_setup _____________________________________________________________________________                                                  

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x7f30cba85c90>                                                                                                                                                               

    def test_wind_setup(self):                                                                                                                                                                                                          
        """Test that winds are set up correctly."""                                                                                                                                                                                     
        mocker = mock.MagicMock()                                                                                                                                                                                                       
        mocker.return_value = True                                                                                                                                                                                                      
        with mock.patch('pycoxmunk.CM_Main.CMSharedWind', mocker):                                                                                                                                                                      
>           with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):                                                                                                                                   
E           Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.                                                                                                                                            
E           The list of emitted warnings is: [].                                                                                                                                                                                        

pycoxmunk/Tests/test_CM_main.py:165: Failed                                                                                                                                                                                             
______________________________________________________________________________ TestCMMain.test_pixmask _______________________________________________________________________________                                                  

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x7f30cba85b10>

    def test_pixmask(self):
        """Test that pixmask is correctly initialised in the class."""
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk/Tests/test_CM_main.py:190: Failed
______________________________________________________________________________ TestCMMain.test_retr_cm _______________________________________________________________________________

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x7f30cba85810>, mock_cmr_func = <MagicMock name='calc_coxmunk_wrapper' id='139847227143648'>

    @mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
    def test_retr_cm(self, mock_cmr_func):
        """Tests for the main routine that retrieves reflectance."""

        cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
        cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

        # This section tests that cox_munk reflectance is computed, but not BRDF
        # Recreate scene in case previous tests have messed with it.
        self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
        mock_cmr_func.return_value = self._make_mocked_cm_refl()
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk/Tests/test_CM_main.py:207: Failed
______________________________________________________________________________ TestCMMain.test_deleter _______________________________________________________________________________

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x7f30cba84940>, mock_cmr_func = <MagicMock name='calc_coxmunk_wrapper' id='139847226750240'>

    @mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
    def test_deleter(self, mock_cmr_func):
        """Test that deletions are done correctly."""

        tmp_arr = np.array([1, 4., 64])

        # Test for case where we don't delete intermediate data
        self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
        mock_cmr_func.return_value = self._make_mocked_cm_refl()
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in double_scalars')].

pycoxmunk/Tests/test_CM_main.py:262: Failed
================================================================================== warnings summary ==================================================================================
pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_badbands
pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_ocean_color_init
pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_ocean_color_init
pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_calcangles
  /tmp/PyCoxMunk/pycoxmunk/CM_Shared_Wind.py:68: RuntimeWarning: invalid value encountered in double_scalars
    self.wd = da.arccos(self.v10 / self.ws)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================================================== short test summary info ===============================================================================
FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_wind_setup - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_pixmask - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_retr_cm - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_deleter - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
===================================================================== 4 failed, 28 passed, 4 warnings in 12.37s ======================================================================

I also have the failing tests (linux Python 3.10)

pdebuyl commented 1 year ago

@arthur-e @molinav would you assess the latest update by @simonrp84 ?

(PS: @simonrp84 would you provide the example output for the example notebooks?)

molinav commented 1 year ago

@pdebuyl I will try to provide my updates during the weekend, it has been a busy time for me lately.

pdebuyl commented 1 year ago

@arthur-e @molinav -> gentle reminder about the review

arthur-e commented 1 year ago

I can verify that all tests pass on the trunk version (1d57ab). However, I found it was necessary to use:

pytest-3 .

As pytest seems to be reserved for the Python 2 version of pytest. Maybe this depends on how pytest is installed. I didn't use conda.

All of my review items are complete and I can recommend PyCoxMunk be accepted.

simonrp84 commented 1 year ago

@arthur-e Thanks for checking. I suspect that may be something to do with the way it's installed. pytest . works OK here (win32, all packages installed via conda).

molinav commented 1 year ago

@simonrp84 I will try to provide my final feedback between today and tomorrow, I just came back from holidays. I will have no objections with having PyCoxMunk being accepted, most of my last points were minor details and I can always report them to you in the PyCoxMunk issue tracker.

simonrp84 commented 1 year ago

Thanks, no worries. I'm also just back from holidays + work travel so am catching up on this now!

pdebuyl commented 1 year ago

Thank you @arthur-e for completing the review!

(thanks @molinav for the update, I'll wait for formal approval here but will start the editorial checks anyway)

simonrp84 commented 1 year ago

Just to add, I plan some minor updates to the example code (fixing a few incorrect comments) and I'll also add the ERA5 data to the repository.

You asked if I could upload the example notebook output, @pdebuyl: I'm sorting that now and plan to put it on zenodo. Will see if I can include the SEVIRI data file used in the notebook too, but need to hear back from EUMETSAT about the license first.

simonrp84 commented 1 year ago

OK, @pdebuyl I have added sample output data for the three example notebooks to a zenodo repository, and have also included an SLSTR scene in the same repo: https://zenodo.org/record/7885996 The notebooks have been updated to point to the repo, and I also used the opportunity to correct a few comments in the notebooks.

I would also like to include SEVIRI data in that, but need to check with EUMETSAT about licensing. Not sure how long that'll take, hopefully it's quick! If not, I can always add SEVIRI to the repo once I have a response from them in the future.

simonrp84 commented 1 year ago

Based on feedback from EUMETSAT, I have now been able to include a SEVIRI dataset on zenodo for use with the example notebook. Have updated the notebooks accordingly.

I also discovered why you have noticed tests failing. I had some tests set up to calculate angles internally, and floating point errors were meaning tests occasionally failed (happened on my laptop, but not my office PC). This should also be corrected in the latest push to main.

molinav commented 1 year ago

@editorialbot generate pdf

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:

molinav commented 1 year ago

I re-checked everything with the latest main branch of pycoxmunk.

The requests in the manuscript have been addressed.
The requests in the GitHub workflow have also been addressed (see [GitHub Workflow]). I think the workflow change to python -m build is not enough here to test against the installed wheel, because the tests are run in the repository root folder and the pycoxmunk folder is in PATH and might take precedence over the installed wheel. Anyway, it is just a detail and I can give you advice outside the paper loop.

[GitHub Workflow]: https://github.com/simonrp84/PyCoxMunk/blob/a0ee002/.github/workflows/python-app.yaml#L38

The requests about the license have been clarified successfully.
About the unit tests, I keep getting errors on Windows (this time with Python 3.11 and pyenv-win Python installation, no conda involved). All errors are about expected warnings not being found. This is similar to what I found in previous iterations. These errors in the unit tests are not as dramatic as if they were caused by unexpected results. Still I would consider important to fix them at some point, because they might be source of corner situations that later might be difficult to debug. If @pdebuyl is ok with it, we can move forward here and we continue iterating in your issue tracker, since I do not know the criterion of JOSS in this situation. I leave the traceback below for reference:

(joss311) [vic@surface] C:\Users\vic\Desktop\scratch> python -m pytest --cov=pycoxmunk --cov-report html PyCoxMunk/pycoxmunk/Tests
========================================================== test session starts ===========================================================
platform win32 -- Python 3.11.2, pytest-7.3.1, pluggy-1.0.0
rootdir: C:\Users\vic\Desktop\scratch
plugins: cov-4.0.0
collected 32 items

PyCoxMunk\pycoxmunk\Tests\test_CMCalcs.py .........                                                                                 [ 28%]
PyCoxMunk\pycoxmunk\Tests\test_CM_main.py ...F.FFFF                                                                                 [ 56%]
PyCoxMunk\pycoxmunk\Tests\test_PixMask.py ..                                                                                        [ 62%]
PyCoxMunk\pycoxmunk\Tests\test_SceneGeom.py ....                                                                                    [ 75%]
PyCoxMunk\pycoxmunk\Tests\test_SharedWind.py .                                                                                      [ 78%]
PyCoxMunk\pycoxmunk\Tests\test_Utils.py ...                                                                                         [ 87%]
PyCoxMunk\pycoxmunk\Tests\test_WaterConsts.py ....                                                                                  [100%] 

================================================================ FAILURES ================================================================ 
_______________________________________________________ TestCMMain.test_calcangles _______________________________________________________ 

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000236D1AB1350>

    def test_calcangles(self):
        """Check that angle calculation is initialised correctly."""
        mocker = mock.MagicMock()
        mocker_lalo = mock.MagicMock()
        mocker_lalo.attrs['area'].get_lonlats = self._get_lonlats
        tmp_dict = {'VIS006': mocker_lalo,
                    'solar_zenith_angle': np.array([10, 10]),
                    'satellite_zenith_angle': np.array([10, 10]),
                    'solar_azimuth_angle': np.array([10, 10]),
                    'satellite_azimuth_angle': np.array([10, 10])}
        mocker.return_value = tmp_dict
        with mock.patch('pycoxmunk.CM_Main.cm_calcangles', mocker):
>           with pytest.warns(RuntimeWarning, match="invalid value encountered in double_scalars"):
E           Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) matching the regex were emitted.
E            Regex: invalid value encountered in double_scalars
E            Emitted warnings: [RuntimeWarning('invalid value encountered in scalar divide')]

PyCoxMunk\pycoxmunk\Tests\test_CM_main.py:170: Failed
_______________________________________________________ TestCMMain.test_wind_setup _______________________________________________________ 

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000236D1AB2310>

    def test_wind_setup(self):
        """Test that winds are set up correctly."""
        mocker = mock.MagicMock()
        mocker.return_value = True
        with mock.patch('pycoxmunk.CM_Main.CMSharedWind', mocker):
>           with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E           Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E           The list of emitted warnings is: [].

PyCoxMunk\pycoxmunk\Tests\test_CM_main.py:185: Failed
________________________________________________________ TestCMMain.test_pixmask _________________________________________________________ 

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000236D1AB0C90>

    def test_pixmask(self):
        """Test that pixmask is correctly initialised in the class."""
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in scalar divide')].

PyCoxMunk\pycoxmunk\Tests\test_CM_main.py:210: Failed
________________________________________________________ TestCMMain.test_retr_cm _________________________________________________________ 

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000236D1AB2FD0>
mock_cmr_func = <MagicMock name='calc_coxmunk_wrapper' id='2434511232848'>

    @mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
    def test_retr_cm(self, mock_cmr_func):
        """Tests for the main routine that retrieves reflectance."""

        cm_band_name = f'cox_munk_refl_{self.good_bnd_names[0]}'
        cm_rho0v_name = f'cox_munk_rho0v_{self.good_bnd_names[0]}'

        # This section tests that cox_munk reflectance is computed, but not BRDF
        # Recreate scene in case previous tests have messed with it.
        self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
        mock_cmr_func.return_value = self._make_mocked_cm_refl()
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in scalar divide')].

PyCoxMunk\pycoxmunk\Tests\test_CM_main.py:227: Failed
________________________________________________________ TestCMMain.test_deleter _________________________________________________________ 

self = <pycoxmunk.Tests.test_CM_main.TestCMMain object at 0x00000236D1AB35D0>
mock_cmr_func = <MagicMock name='calc_coxmunk_wrapper' id='2434506064080'>

    @mock.patch('pycoxmunk.CM_Main.calc_coxmunk_wrapper')
    def test_deleter(self, mock_cmr_func):
        """Test that deletions are done correctly."""

        tmp_arr = np.array([1, 4., 64])

        # Test for case where we don't delete intermediate data
        self.scn_good = self.create_test_scene(self.good_bnd_names, self.good_angle_names)
        mock_cmr_func.return_value = self._make_mocked_cm_refl()
>       with pytest.warns(UserWarning, match="Some solar zenith values out of range. Clipping."):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E       The list of emitted warnings is: [RuntimeWarning('invalid value encountered in scalar divide')].

PyCoxMunk\pycoxmunk\Tests\test_CM_main.py:282: Failed

---------- coverage: platform win32, python 3.11.2-final-0 -----------
Coverage HTML written to dir htmlcov
======================================================== short test summary info =========================================================       
FAILED PyCoxMunk/pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_calcangles - Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) matching the regex were emitted.
FAILED PyCoxMunk/pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_wind_setup - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED PyCoxMunk/pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_pixmask - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED PyCoxMunk/pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_retr_cm - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
FAILED PyCoxMunk/pycoxmunk/Tests/test_CM_main.py::TestCMMain::test_deleter - Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
===================================================== 5 failed, 27 passed in 25.25s ======================================================

About Simple_GOES_ABI.ipynb: working as before. One typo was not corrected from the previous iteration:
- [ ] "We do not apply and cloud or land masking" => "We do not apply any cloud or land masking".
About Simple_SLSTR.ipynb: now working. Here I used again the file that I downloaded, "S3A_SL_1_RBT____20230203T041025_20230203T041325_20230204T123946_0179_095_104_3060_PS1_O_NT_004.SEN3". Same typo correction applies as before:
- [ ] "We do not apply and cloud or land masking" => "We do not apply any cloud or land masking".
About Complex_SEVIRI.ipynb:
- netCDF4 needs to be indicated as additional library to install for this example, since it is not installed as a dependency of pycoxmunk and the user might not have it.
- It seems that xarray==2023.3.0 is the last version of xarray that can identify rasterio as a backend to open generic image files. From version xarray==2023.4.0, this is not the case anymore (see xarray release notes for v2023.4.0) and the IPython notebook will fail when trying to open the SEVIRI land mask in TIFF format. It seems that this backend was deprecated for some time and it just happened that they removed this functionality during the review loop of your paper. I do not have a solution for that, as I am not an xarray regular user. If I test the notebook skipping the land mask loading and the pcm.setup_pixmask call, I can reach the end of the notebook and save the output files, but the results differ of course.

TLDR summary comes in a following comment.

molinav commented 1 year ago

So my overall summary would be the following:

The purpose of the library is clear, the base functionality is clear, the licensing is clear, the packaging is correct.
The GitHub workflow would need minor tweaking to ensure that the tests are run for the installed wheel.
The GitHub workflow could add in the future a matrix of Python versions and operating systems to avoid having unexpected behaviours (e.g. why sometimes the unit tests do not throw the expected warnings).
The three example notebooks essentially work. They would need a bit of improvement so that they clearly state the extra dependencies that are required. Also, the notebooks define specific paths based on the "D:" Windows drive, which force the user to tune the notebooks before they can actually work (e.g. these paths are not valid for a GNU/Linux user). Other issues are coming from changes in external libraries, and this is out of control of pycoxmunk.

If I had to decide, I would say the pycoxmunk paper can be accepted now as it is. All these corrections coming from me are in the end minor corrections, and if pycoxmunk keeps alive they would get fixed sooner or later. No 1.0.0 library version is perfect anyway, and @simonrp84 has been actively updating the library based on the feedback received here, so I am sure that the remaining issues will eventually be solved in the corresponding GitHub repo.

simonrp84 commented 1 year ago

Thanks @molinav! Not sure how I overlooked those typos in the notebooks. Fixed now. I also added a note to the SEVIRI notebook saying that netCDF4 is required. I also added a note to all three notebooks discussing the directories, and changed the default directory from D:/sat_data/... to ./, which should be cross-platform.

Regarding your xarray comment: This is an issue in satpy, we released a new version last week (0.42.x) that should fix it, so I guess you're using a slightly older version of satpy. I just tried with a fresh environment and it works OK with the latest versions.

The unit tests issue is interesting - I've been testing on python <3.10 and actually installed 3.11 for the first time today. Running the tests now I can reproduce your failed warnings so presumably something has changed in the new python version. I'll investigate that...and agree it'd be good to test in CI against multiple versions.

simonrp84 commented 1 year ago

Ok, I fixed the tests - it was my use of np.random that was causing the problem: I thought a 10x10 array would be enough to ensure that a value would be present to trip the condition but apparently not. I'm now explicitly setting the array values so there'll always be some that raise the warning.

I also added a warning for the invalid divide that was mentioned earlier. Because the actual warning message differs between 3.10 and 3.11 I couldn't check the warning text with a regex, so it's a generic RuntimeWarning.

molinav commented 1 year ago

Hi @simonrp84! I can also confirm that the tests are passing for me after your last changes. See traceback below:

(joss311) [vic@onyx] C:\Users\vic\Desktop\scratch> python -m pytest --cov=pycoxmunk --cov-report=html .\libraries\PyCoxMunk\pycoxmunk\Tests
================================================================================= test session starts ================================================================================== 
platform win32 -- Python 3.11.2, pytest-7.3.1, pluggy-1.0.0
rootdir: C:\Users\vic\Desktop\scratch
plugins: cov-4.0.0
collected 32 items

libraries\PyCoxMunk\pycoxmunk\Tests\test_CMCalcs.py .........                                                                                                                     [ 28%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_CM_main.py .........                                                                                                                     [ 56%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_PixMask.py ..                                                                                                                            [ 62%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_SceneGeom.py ....                                                                                                                        [ 75%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_SharedWind.py .                                                                                                                          [ 78%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_Utils.py ...                                                                                                                             [ 87%] 
libraries\PyCoxMunk\pycoxmunk\Tests\test_WaterConsts.py ....                                                                                                                      [100%] 

---------- coverage: platform win32, python 3.11.2-final-0 -----------
Coverage HTML written to dir htmlcov

As you found out, using numpy.random.uniform was problematic for the reproducibility. If numpy.random is not used with a fixed predefined seed, the unit tests might be working with different data depending on how the default seed is being generated.

The notebooks for GOES and SLSTR now work without any extra work apart from dropping the downloaded files in the correct place inside the Examples folder. For the SEVIRI example I keep getting the former error:

ValueError                                Traceback (most recent call last)
Cell In[7], line 2
      1 # Load the land mask
----> 2 lsm_scn = Scene([lsm_fname], reader='generic_image')
      3 lsm_scn.load(['image'])

File c:\Users\vic\Desktop\scratch\joss311\Lib\site-packages\satpy\scene.py:133, in Scene.__init__(self, filenames, reader, filter_parameters, reader_kwargs)
    130 if filenames:
    131     filenames = convert_remote_files_to_fsspec(filenames, storage_options)
--> 133 self._readers = self._create_reader_instances(filenames=filenames,
    134                                               reader=reader,
    135                                               reader_kwargs=cleaned_reader_kwargs)
    136 self._datasets = DatasetDict()
    137 self._wishlist = set()

File c:\Users\vic\Desktop\scratch\joss311\Lib\site-packages\satpy\scene.py:154, in Scene._create_reader_instances(self, filenames, reader, reader_kwargs)
    149 def _create_reader_instances(self,
    150                              filenames=None,
    151                              reader=None,
    152                              reader_kwargs=None):
    153     """Find readers and return their instances."""
--> 154     return load_readers(filenames=filenames,
    155                         reader=reader,
    156                         reader_kwargs=reader_kwargs)
...
    207         )
    208     backend = engines[engine]
    209 elif isinstance(engine, type) and issubclass(engine, BackendEntrypoint):

ValueError: unrecognized engine rasterio must be one of: ['netcdf4', 'h5netcdf', 'scipy', 'store', 'zarr']

It seems I should be up to date with the dependencies, but anyway I am not an expert on satpy and related libraries:

(joss311) [vic@onyx] C:\Users\vic\Desktop\scratch> python -c "import sys; [__import__(lib) and print('{0}: {1}'.format(lib, sys.modules[lib].__version__)) for lib in ('satpy', 'rasterio', 'xarray')]"
satpy: 0.42.1
rasterio: 1.3.6
xarray: 2023.4.2

simonrp84 commented 1 year ago

Aah, silly me - there's an additional dependency for this, you need to install rioxarray!

I'll add that to the note at the top of the SEVIRI example.

(edit) As you suggested yesterday, I've also now added multiple python versions to github actions.

molinav commented 1 year ago

Now the SEVIRI example also works completely for me, it was the missing rioxarray as you indicated. There are no more comments from my side. Nice work!

simonrp84 commented 1 year ago

Great, thanks for the feedback- much appreciated.

simonrp84 commented 1 year ago

Ping @pdebuyl - I think I've covered all that's been requested, anything else I need to do before this is ready?

pdebuyl commented 1 year ago

Nothing to do on your side yet @simonrp84 I'll get to it this week.

pdebuyl commented 1 year ago

Thank you @molinav for completing the review!

pdebuyl commented 1 year ago

@editorialbot generate pdf

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:

pdebuyl commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.5194/amt-3-813-2010 may be a valid DOI for title: A sea surface reflectance model for (A) ATSR, and application to aerosol retrievals
- 10.5194/amt-5-1889-2012 may be a valid DOI for title: Cloud retrievals from satellite data using optimal estimation: evaluation and application to ATSR
- 10.1364/josa.44.000838 may be a valid DOI for title: Measurement of the roughness of the sea surface from photographs of the sun’s glitter
- 10.1175/bams-d-17-0277.1 may be a valid DOI for title: Pytroll: An open-source, community-driven python framework to process earth observation satellite data
- 10.25080/majora-7b98e3ed-013 may be a valid DOI for title: Dask: Parallel computation with blocked algorithms and task scheduling
- 10.1016/j.jqsrt.2009.10.001 may be a valid DOI for title: Evaluation of sun glint models using MODIS measurements

INVALID DOIs

- None

simonrp84 commented 1 year ago

@editorialbot generate pdf

openjournals / joss-reviews

[REVIEW]: pycoxmunk: A python package for computing sea surface reflectance #5074

Status

Reviewer instructions & questions

Checklists

Review checklist for @arthur-e

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @molinav

Conflict of interest

Code of Conduct

General checks

Functionality

The CM_Shared_Wind Module