Analysis mode is now also derived from non-str sources by forman · Pull Request #44 · EOPF-Sample-Service/xarray-eopf

forman · 2025-04-15T14:37:12Z

Analysis mode is now also derived from non-str sources.

Closes #40

codecov · 2025-04-15T14:38:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
xarray_eopf/amode.py	`100.00% <100.00%> (ø)`
xarray_eopf/amodes/sentinel2.py	`100.00% <100.00%> (ø)`
xarray_eopf/backend.py	`97.77% <100.00%> (+1.48%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

konstntokas

Everything looks good to me.

Please consider my suggestions below — it would be helpful if you could elaborate on my comments. That said, they’re all minor, so I'm happy to approve with suggestions.

konstntokas · 2025-04-16T05:40:20Z

+            product_type: Optional product type name, such as `"MSIL1C"`.
+                Only used if `op_mode="analysis"`. You should not need
+                pass this argument, if the filename inherent to `filename_or_obj`
+                adheres to EOPF naming conventions.


Suggested change

product_type: Optional product type name, such as `"MSIL1C"`.

Only used if `op_mode="analysis"`. You should not need

pass this argument, if the filename inherent to `filename_or_obj`

adheres to EOPF naming conventions.

product_type: Optional product type name, such as `"MSIL1C"`.

Only used if `op_mode="analysis"`; typically not required if

`filename_or_obj` follows EOPF naming conventions.

I prefer staying with my version as filename_or_obj may be some other object which cannot directly follow a naming convention, however its path or root attribute can.

But I like the semicolon.

konstntokas · 2025-04-16T05:43:54Z

+            product_type: Optional product type name, such as `"MSIL1C"`.
+                Only used if `op_mode="analysis"`. You should not need
+                pass this argument, if the filename inherent to `filename_or_obj`
+                adheres to EOPF naming conventions.


Suggested change

product_type: Optional product type name, such as `"MSIL1C"`.

Only used if `op_mode="analysis"`. You should not need

pass this argument, if the filename inherent to `filename_or_obj`

adheres to EOPF naming conventions.

product_type: Optional product type name, such as `"MSIL1C"`.

Only used if `op_mode="analysis"`; typically not required if

`filename_or_obj` follows EOPF naming conventions.

konstntokas · 2025-04-16T06:05:36Z

+        self.assertIsInstance(AnalysisMode.guess("TEST.zarr"), TestMode)
+        self.assertIsInstance(AnalysisMode.guess({}, product_type="TEST"), TestMode)
+        self.assertIsInstance(
+            AnalysisMode.guess("TEST.zarr", product_type="REST"), TestMode


This behavior isn't intuitive to me. It first tries to guess the analysis mode from the product type "REST", which fails, and then proceeds to derive it from "TEST.zarr". I would have expected it to fail outright if the first guess didn't succeed.

That said, perhaps there's a reason you chose to implement it using two separate if statements instead of an if–elif structure?

I agree. If product_type is provided, it should be used and fail if it does not succeed. Will change this. Thanks!

konstntokas · 2025-04-16T06:09:32Z

+            fsspec.filesystem("local").get_mapper("test3.zarr")
+        )
+        self.assertIsInstance(path, str)
+        self.assertEqual("test3.zarr", Path(path).name)


Suggested change

self.assertEqual("test3.zarr", Path(path).name)

self.assertEqual("test3.zarr", path)

Or am I missing something?

Fine! Copy/paste.

Nope, it was correct! Path(path).name gets the filename of an absolute path.

konstntokas · 2025-04-16T06:10:18Z

+        # From zarr.storage.DirectoryStore
+        path = AnalysisMode._source_to_path(zarr.storage.DirectoryStore("test4.zarr"))
+        self.assertIsInstance(path, str)
+        self.assertEqual("test4.zarr", Path(path).name)


Suggested change

self.assertEqual("test4.zarr", Path(path).name)

self.assertEqual("test4.zarr", path)

See comment above

Same here, it is correct.

Co-authored-by: Konstantin Ntokas <38956538+konstntokas@users.noreply.github.com>

Analysis mode is now also derived from non-str sources

8133372

forman added 2 commits April 15, 2025 17:58

improved error message

bec27af

improved docstrings

a53e73a

forman requested a review from konstntokas April 15, 2025 15:59

forman marked this pull request as ready for review April 15, 2025 15:59

konstntokas approved these changes Apr 16, 2025

View reviewed changes

forman and others added 6 commits April 16, 2025 08:43

Update xarray_eopf/backend.py

9f28485

Co-authored-by: Konstantin Ntokas <38956538+konstntokas@users.noreply.github.com>

Update xarray_eopf/backend.py

69a556d

Co-authored-by: Konstantin Ntokas <38956538+konstntokas@users.noreply.github.com>

Update tests/test_amode.py

fa0aa4a

Co-authored-by: Konstantin Ntokas <38956538+konstntokas@users.noreply.github.com>

Update tests/test_amode.py

867eca5

Co-authored-by: Konstantin Ntokas <38956538+konstntokas@users.noreply.github.com>

improved docstring

9ab8f44

fixed logic

4f8d3c7

forman requested a review from konstntokas April 16, 2025 07:19

konstntokas approved these changes Apr 16, 2025

View reviewed changes

forman merged commit 2cccebc into main Apr 16, 2025
2 checks passed

forman deleted the forman-40-detect_analysis_mode_from_product_type branch April 16, 2025 07:56

	self.assertEqual("test3.zarr", Path(path).name)
	self.assertEqual("test3.zarr", path)

	self.assertEqual("test4.zarr", Path(path).name)
	self.assertEqual("test4.zarr", path)

Conversation

forman commented Apr 15, 2025

Uh oh!

codecov Bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

konstntokas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 15, 2025 •

edited

Loading