catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

[Refactor] standardize sorting interface kwargs #525

Closed CodyCBakerPhD closed 2 years ago

CodyCBakerPhD commented 2 years ago

Continuation of #516 for sorting interfaces.

CodyCBakerPhD commented 2 years ago

@h-mayorquin The main reason I have to set verbose=False in all the gallery docs is because it otherwise expects no output when the NWBFile saved at... message outputs. Is it possible to tell the output of >>> blocks to expect some type of regular expression? (the exact name of the files in the tests belongs to a temporary directory, and local output would of course differ from one person's system to the next)

h-mayorquin commented 2 years ago

@h-mayorquin The main reason I have to set verbose=False in all the gallery docs is because it otherwise expects no output when the NWBFile saved at... message outputs. Is it possible to tell the output of >>> blocks to expect some type of regular expression? (the exact name of the files in the tests belongs to a temporary directory, and local output would of course differ from one person's system to the next)

Not as far as I know. I think that we should keep verbose=False here. Another option might be to we use the skiptest functionality: https://docs.pytest.org/en/6.2.x/doctest.html#skipping-tests

CodyCBakerPhD commented 2 years ago

Another option might be to we use the skiptest functionality: https://docs.pytest.org/en/6.2.x/doctest.html#skipping-tests

But wouldn't skipping it prevent the line from getting run (since the results of that line are needed for the next lines)?

I'd prefer to not have to specify it at all in the minimal copy/paste examples since it begs the question of why we're changing it there to begin with.

codecov[bot] commented 2 years ago

Codecov Report

Merging #525 (e18b378) into main (fa98352) will decrease coverage by 0.09%. The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
- Coverage   87.84%   87.74%   -0.10%     
==========================================
  Files          58       58              
  Lines        3004     3004              
==========================================
- Hits         2639     2636       -3     
- Misses        365      368       +3     
Flag Coverage Δ
unittests 87.74% <85.71%> (-0.10%) :arrow_down:

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

Impacted Files Coverage Δ
...aces/ecephys/neuroscope/neuroscopedatainterface.py 75.63% <ø> (ø)
...rsion_tools/tools/spikeinterface/spikeinterface.py 85.71% <70.00%> (-0.66%) :arrow_down:
...nterfaces/ecephys/basesortingextractorinterface.py 80.76% <100.00%> (+0.76%) :arrow_up:
.../ecephys/cellexplorer/cellexplorerdatainterface.py 79.22% <100.00%> (ø)
...terfaces/ecephys/kilosort/kilosortdatainterface.py 100.00% <100.00%> (ø)
...ols/datainterfaces/ecephys/phy/phydatainterface.py 100.00% <100.00%> (ø)
...s/ecephys/tutorial/sortingtutorialdatainterface.py 100.00% <100.00%> (ø)
CodyCBakerPhD commented 2 years ago

It seems that it was a lot of busy work to "verbosify" everything

Yeah, almost regretting making it optional.