Skip to content

Drop the Dataset Statistics section when no chart actually renders - #399

Open
adityasingh2400 wants to merge 2 commits into
eegdash:developfrom
adityasingh2400:fix/378-empty-dataset-statistics-section
Open

Drop the Dataset Statistics section when no chart actually renders#399
adityasingh2400 wants to merge 2 commits into
eegdash:developfrom
adityasingh2400:fix/378-empty-dataset-statistics-section

Conversation

@adityasingh2400

Copy link
Copy Markdown

Closes #378.

The section already had a guard meant to collapse it when there is nothing to show, but it counted appended parts instead of appended content. The channel count and sampling frequency renderers are appended unconditionally once their has_ flag is set, and _make_count_bar_chart returns an empty string when the entries it was handed carry no usable val. That empty append still grew parts, so len(parts) <= 1 was False and the page kept a Dataset Statistics heading above an empty cohort grid.

Testing the joined content instead makes the section collapse for a dataset with no usable subject, age, channel count or sampling frequency metadata, which is the case in the issue. The change is one line in _format_recording_stats_section plus a comment explaining why it tests content rather than length.

The issue body is empty, so I traced the symptom to the code rather than working from a description. Added tests/test_dataset_page_stats.py with six cases, following the section_module fixture pattern already used by test_dataset_page_electrodes.py. Three of them fail on develop, and the failure diff prints exactly the reported symptom, a Dataset Statistics heading followed by an empty eegdash-ed-cohort-grid. The other two go the other direction, checking that a dataset with real channel counts or a real duration still renders, so the guard does not overcorrect.

After the change, 11 pass, my 6 plus the 5 existing dataset_page tests. The repo's own pre-commit ruff args pass clean and ruff format --check reports both files already formatted.

Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.

The section guard counted appended parts instead of appended content.
The nchans and sfreq renderers are appended unconditionally once their
has_* flag is set, and they return an empty string when the entries they
were handed carry no usable val. That empty append still grew parts, so
len(parts) <= 1 was False and the page kept a Dataset Statistics heading
above an empty cohort grid.

Test the joined content instead, so a dataset with no usable subject,
age, channel-count or sampling-frequency metadata collapses the section
entirely.

Closes eegdash#378
@adityasingh2400

Copy link
Copy Markdown
Author

Two things about the red here, and the second one is a problem with my own PR rather than with CI.

The failures are a network outage, not this change. All 21 in test (ubuntu-latest, 3.12) are ConnectTimeout against data.eegdash.org, for example:

requests.exceptions.ConnectTimeout: HTTPSConnectionPool(host='data.eegdash.org', port=443):
Max retries exceeded with url: /api/eegdash/count
(Caused by ConnectTimeoutError(..., 'Connection to data.eegdash.org timed out. (connect timeout=30)'))

docs (ubuntu-latest, 3.12) fails for the same reason, one layer up: sphinx_gallery.gen_gallery raises ExtensionError from that same timeout during builder-inited, so make html never gets to render anything. This PR touches _format_recording_stats_section, which does no network access, and the run took 2 hours 28 minutes, almost all of it in connect timeouts.

The part worth acting on: my tests are not running. The job reports

SKIPPED [1] tests/test_dataset_page_stats.py:22: dataset_page extension requires sphinx

so the 106 lines I added are skipped in the test job, which does not install sphinx, and the docs job builds documentation rather than running pytest. That means this PR currently has no CI coverage at all for the behavior it changes, which is not what I intended when I added the file.

I would rather fix that than leave a test that only ever passes locally. Before I pick an approach, which would you prefer:

  1. Drop the sphinx dependency from the test by exercising _format_recording_stats_section directly with plain dictionaries, so it runs in the ordinary test job.
  2. Add sphinx to the test environment so the existing test executes as written.
  3. Leave it skipped, if the docs extension is deliberately outside the unit test scope here.

Happy to implement whichever fits how you want this directory covered.

The test job installs the package without doc dependencies, so
importorskip("sphinx") skipped this whole file and the PR had no CI
coverage. sections.py touches sphinx once, for a module logger, so a
stdlib logger stands in and the functions under test are unchanged.
@adityasingh2400

Copy link
Copy Markdown
Author

Went with option 1 since it is the only one that needs nothing from you, and leaving the file skipped meant this PR had no coverage at all. Say the word if you would rather have option 2 or 3 and I will switch.

sections.py touches sphinx exactly once, LOGGER = logging.getLogger(__name__) against sphinx.util.logging, and the functions under test never log. So instead of skipping, the module now stands in a stdlib logger when sphinx is absent. The code under test is untouched.

Verified in a venv built the way the test job builds one, pip install -e . with no doc extras: 6 passed. With sphinx installed it is also 6 passed, so the docs path is unaffected. Reverting sections.py to the pre-fix version fails 3 of the 6, so the coverage is real rather than vacuous.

One thing you may want to look at separately: tests/test_dataset_page_electrodes.py uses the same pytest.importorskip("sphinx") line, so it is also being skipped in the test job today. I left it alone since it is outside this PR, but it is the same gap.

The 21 ConnectTimeout failures are unrelated and still present. data.eegdash.org is still refusing connections as of a few minutes ago, TCP never establishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Once the metainformation for subject and age is not present, the data statistic section (Dataset Statistics) should not be added.

1 participant