feat(melodi): tests, docstrings, fix measures parsing by GrasClement · Pull Request #308 · InseeFrLab/pynsee

GrasClement · 2026-06-15T19:35:44Z

Draft contribution aux JDOS : cible la branche #306

Fix _parse_dataset_observations (voir package R Melodi)
Docstrings manquantes
Tests des 4 fonctions publiques

Signed-off-by: GrasClement <gras.clement@orange.fr>

GrasClement · 2026-06-17T15:46:44Z

Hi,

The JDOS are done, here is a quick closure summary for this PR,

Bug Fixes

Fixed hardcoded measure key in _parse_dataset_observations
Added guard for observations without "measures" field (prevents crashes)
Corrected language filter logic in get_idbank
Improved handling of non-MELODI idbanks → returns empty DataFrame

New Content

Docstrings for 4 public MELODI functions + 2 internal helpers
Tests: 14 tests covering parsing, edge cases, and all public functions
Example notebook: example_internet_usage_melodi.ipynb
Updated MELODI docs integration and DDL deprecation warning

Code Maintenance

Added update and silent parameters to all 4 public functions
Updated imports and version (0.2.6)
Updated .gitignore for development files

Cheers

tfardet · 2026-06-19T19:04:19Z

Thanks @GrasClement !
@tgrandje may I let you have a look at this since it targets your previous PR?
I'm not sure I'll have the time to look at this any time soon...

tgrandje · 2026-07-04T11:59:25Z

Hi @GrasClement , sorry I took so long to review this, I was overloaded at work.

So I finally took the time to do it, with the exception of testing the documentation (I'm in a train on a personnal windows machine...) though it looks ok to me. I've made some comments: depending of your available time, feel free to let me know if you haven't the time to check all of this.

In any case, thank you for both your great work and the effort to uniformize the API with the silent/update kwargs ❤️ !

GrasClement · 2026-07-09T10:18:29Z

Hi @tgrandje, no worries about the timing, and thanks for taking the time to review the PR!

I don't seem to see any review comments or additional commits attached to the PR yet. Whenever you have a chance, could you check whether the comments were actually submitted? Or maybe the issue is on my side and I'm looking in the wrong place.

I'd be happy to go through your feedback and update the PR once this is sorted out 😎

tgrandje

Sorry @GrasClement I forgot to submit the review 😳 !

tgrandje · 2026-07-04T10:13:29Z

+        # déplie toutes les clés de mesure dynamiquement
+        # (le nom varie selon le dataset : OBS_VALUE_NIVEAU, OBS_VALUE_INDICE_DE_PRIX, etc.)


Should we switch the comments to english?

tgrandje · 2026-07-04T10:27:30Z

+        measures_expanded = pd.DataFrame(
+            obs["measures"]
+            .apply(lambda m: {k: v.get("value") for k, v in m.items()})
+            .tolist()
+        )


Isn't the key unique in a dataset? If that's the case maybe we could avoid the whole apply process, what do you think?

tgrandje · 2026-07-04T10:57:58Z

@@ -664,8 +764,9 @@ def get_idbank(
    # test = get_range("DS_TICM_PRATIQUES", include_values=True)


While you're at it, would you mind removing the tailing tests under the if __name__ == "__main__" block?

tgrandje · 2026-07-04T11:14:29Z

+# Exception here: the bugs fixed in melodi/data/py live inside parsing helpers
+# called inside get_dataset. Testing them through the real API would
+# require downloading DS_IPC_PRINC on every test run.


Thanks for the explanations! As pynsee was never (yet) released with MELODI api, what do you think of removing the comments detailing the tests' purposes as fixes to my original branch? In case you're ok, I'll flag the involved comments I've seen.

tgrandje · 2026-07-04T11:18:16Z

+
+    def test_get_catalog_known_datasets_present(self):
+        # dataset_identifier is not unique (one dataset can have multiple products).
+        # Assert presence of stable, long-running datasets rather than len > 0.


It took me a while to understand what you meant with the "rather tan len > 0". Maybe we could also remove that part, as it doesn't bring anything useful (I think): I let you be the judge on that 😉 ! (Note: it might very well just me being daft...)

tgrandje · 2026-07-04T11:28:20Z

+        self.assertTrue(test)
+
+    def test_parse_observations_no_measures_column(self):
+        # Regression: observations without "measures" must not crash.


Update commentary in context with pynsee's release with brand new MELODI api? (maybe just remove the regression part?)

tgrandje · 2026-07-04T11:31:11Z

+
+    def test_get_idbank_series_history_stable(self):
+        # 010770930 = DS_ICA monthly series, sector 46.19A (non-food purchasing
+        # centres), base 2021. Open series: do NOT assert len (grows monthly).


I get it we shouldn't check the len, but that does'nt mean we couldn't check wether it's still updated. I was thinking of something like (df.groupby(df["TIME_PERIOD"].str[:4])["dataset"].count()[:-1]==12).all(), what do you think?

tgrandje · 2026-07-04T11:31:53Z

+        # Regression: "key2 in {'all', language}" was backwards: _fr columns
+        # were absent when language="fr". Fixed to "language in {'all', key2}".


Update commentary in context with pynsee's release with brand new MELODI api?

tgrandje · 2026-07-04T11:35:13Z

+
+    def test_get_idbank_dead_bdm_ids(self):
+        # BDM series with no MELODI dataset yet -> HTTP 200 + empty list -> empty df.
+        df = get_idbank("001565530+001565531")


I'm thinking this would be better yet to do something quite explicit with a df = get_idbank("spam+eggs"); for those of us outside INSEE, it's not the most convenient of things to retrieve idbanks and check if they have associated data 😉 . By the way, should we log a warning stating that no data was retrieved and that is not a pynsee error?

tgrandje · 2026-07-04T11:50:28Z

+
+MELODI
+------
+
+.. automodule:: pynsee.melodi
+    :members:


What do you think of putting MELODI before utils? I'd even go as far as setting it where METADATA was (and setting METADATA at the latest position in regard of it's deprecation).

cgras added 2 commits June 15, 2026 21:14

add .vscode to gitignore

b6312ea

add notebooks to gitignore

ed8a21c

GrasClement mentioned this pull request Jun 15, 2026

Proposition pour les Journées open-source Insee (16-17 juin 2026) : contribution à pynsee #307

Closed

GrasClement added 8 commits June 16, 2026 16:50

wip docstrings and fixes data.py

3cfcd99

fix measures column issue in _parse_dataset_observations

16d399a

fix(melodi): handle non-MELODI idbanks

6b1f2c1

test(melodi): add pytest coverage and fix update/silent signatures

7551e94

Signed-off-by: GrasClement <gras.clement@orange.fr>

test(melodi): finish tests for melodi

e27041a

Signed-off-by: GrasClement <gras.clement@orange.fr>

doc(melodi): add melodi to the documentation

dc0ee68

Signed-off-by: GrasClement <gras.clement@orange.fr>

doc(melodi): linting

c8d0f7c

Signed-off-by: GrasClement <gras.clement@orange.fr>

doc(melodi): fix spacing

0c1a756

Signed-off-by: GrasClement <gras.clement@orange.fr>

GrasClement marked this pull request as ready for review June 17, 2026 15:33

tgrandje requested changes Jul 9, 2026

View reviewed changes

		# déplie toutes les clés de mesure dynamiquement
		# (le nom varie selon le dataset : OBS_VALUE_NIVEAU, OBS_VALUE_INDICE_DE_PRIX, etc.)

		@@ -664,8 +764,9 @@ def get_idbank(
		# test = get_range("DS_TICM_PRATIQUES", include_values=True)

		# Regression: "key2 in {'all', language}" was backwards: _fr columns
		# were absent when language="fr". Fixed to "language in {'all', key2}".

Uh oh!

Conversation

GrasClement commented Jun 15, 2026

Uh oh!

GrasClement commented Jun 17, 2026

Bug Fixes

New Content

Code Maintenance

Uh oh!

tfardet commented Jun 19, 2026

Uh oh!

tgrandje commented Jul 4, 2026

Uh oh!

GrasClement commented Jul 9, 2026

Uh oh!

tgrandje 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants