Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation example for DecompositionSeries #1981

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

stephprince
Copy link
Contributor

@stephprince stephprince commented Nov 7, 2024

Motivation

Fix #1938.

Note that this example is pending these schema updates: NeurodataWithoutBorders/nwb-schema#593, which make band_mean and band_std optional.

How to test the behavior?

cd docs
make clean && make html

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff check . && codespell from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.04%. Comparing base (084aa55) to head (b3a7934).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1981   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files          27       27           
  Lines        2704     2704           
  Branches      705      705           
=======================================
  Hits         2489     2489           
  Misses        141      141           
  Partials       74       74           
Flag Coverage Δ
integration 72.92% <ø> (ø)
unit 82.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@ehennestad
Copy link

Hi @stephprince. I replicated this example in the ecephys tutorial for matnwb and then I ran nwbinspector on the output file. The nwbinspector errored out before generating the results with the following error:
TypeError: 'MissingDataType' object is not iterable.
The nwbinspector works fine prior to adding the DecompositionSeries to the nwb file.

I thought it was something matnwb specific, but got the same issue when running nwb inspector on the output of the pynwb tutorial with the new DecompositionSeries example. Could you check if you see the same?

@stephprince
Copy link
Contributor Author

stephprince commented Nov 14, 2024

@ehennestad which version of the schema were you using?

I'm waiting to merge this until we release version 2.8.0 of the schema, because this current example will generate invalid files until band_mean and band_stdev are made to be optional columns on the DecompositionSeries.bandstable.

I get the same error as you with the current example + schema, but the inspector should run ok on the file if you change the code to be:

for band_name, band_limits in bands.items():
    decomp_series.add_band(band_name=band_name, band_limits=band_limits, band_mean=1.0, band_stdev=1.0)

but I don't think band_mean or band_stdev are applicable in many cases so ideally we would wait until the next schema release (which should be happening in the next week).

@ehennestad
Copy link

I was using v2.7.0. Thanks for pointing that out again about the extra required properties. I had tried to add them, but found a typo and after correcting that, it works!

@stephprince stephprince marked this pull request as ready for review November 19, 2024 18:43
@stephprince
Copy link
Contributor Author

@rly I've updated the example to add the required band_mean and band_stdev columns as nans so that the tutorial generates valid files using schema v2.7.0 (though it violates best practice suggestions by having a column of all nans).

Once schema v2.8.0 is released, we can update this tutorial to remove those optional columns.

@stephprince stephprince requested a review from rly November 19, 2024 18:51
@stephprince stephprince merged commit 133a2c4 into dev Nov 19, 2024
24 of 25 checks passed
@stephprince stephprince deleted the add-decomposition-docs branch November 19, 2024 19:18
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.

[Documentation]: add documentation example for DecompositionSeries
3 participants