-
Notifications
You must be signed in to change notification settings - Fork 23
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
Second fix for NWB GUIDE nested converter stubbing #979
Conversation
@h-mayorquin OK tests passing now, and left comments in the code explaining routing pattern for different styles of subconverters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see some examples to think better about this.
The example is the SpikeGLXConverterPipe, the source of the issue from GUIDE: NeurodataWithoutBorders/nwb-guide#870 |
Can you add some tests? I think I would need that to judge this well. |
Sure, I can try |
tests/test_on_data/test_format_converters/test_spikeglx_converter.py
Outdated
Show resolved
Hide resolved
source_data = dict(SpikeGLX=dict(folder_path=str(SPIKEGLX_PATH / "Noise4Sam_g0"))) | ||
converter = TestConverter(source_data=source_data) | ||
|
||
conversion_options = dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Let me think a bit on this.
One question:
Do you expect the user to know that the conversion options of each of the sub interfaces are set with their stream names? if so, how how would they? Or are they supposed to be handled by automatic tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All information for specifying the conversion_options
dictionary is available in the conversion_options_schema
So a user has several options:
a) Read the JSON schema returned by the highest-level converter.get_conversion_options_schema()
. Not the easiest thing to do but this is the intended route (principle applies to source data schema and metadata schema too)
b) If they defined a custom NWBConverter
to be nested inside another, they already know their own key references by definition
c) If they used a ConverterPipe
or core-supported subconverter based on a ConverterPipe
, then they could find the automatically generated key references via minimal syntax converter[subconverter_key].data_interface_objects.keys()
. We could even expose a convenience retrieval for it.
The GUIDE of course simplifies all this by automating construction based on the schema structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks a lot fo bearing with me so far.
Second question:
Can we just pass conversion_options to all the interfaces os a pipe? Here in this case, they will be the same, right? Is there a case where they will differ?
I know that they can differ in practice, like, one could theoretically want a different writing buffer or progress bar for one interface of a sub-converter but my question is: is there a place where it would be critical that they are separated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at bruker/miniscope for that style. There, one common option specifies stubbing. Which is mildly convenient for that but therefore the GUIDE does not support in-file progress updates for those converters, whereas it does for SpikeGLX (because we then have control over each TQDM bar for each data stream in the converter)
This is exactly the kind of question I think is fine for you to consider in the long-term; but for now we really just need this bug fixed and released for one of the more popular formats and uses on the GUIDE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then there is no need to generalize yet.
Move the modified add_to_nwb
in this branch from to the SpikeGLX converter. This will fix your bug and will avoid us to modify one of the most central functions in the libraryfor a specific case. If we find a better way in the future then we only need to break the SipkeGLX converter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a minimal modification if it modifies add_to_nwb
of the NWBConverter
. Modifying the converter that triggered the bug directly (SpikeGLX) is the minimal fix as I suggested. What's wrong with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What minimal fix to SpikeGLXConverter are you suggesting that satisfies all the other concerns about its broader use (in particular progress updates)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function that you added here add_to_nwb
.
Add it the converters that need it. That's a localized change.
I thought that it was only spikeglx but it sems that ther is more of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main converter class that needs it.
As demonstrated in the test, this is for an NWBConverter
that contains a SpikeGLXConverter
.
There is no fix to the SpikeGLXConverter
specifically outside of the general standardization of how to style subconverter conversion options, which is a broader question if it should be standardized at all, and if so which standard should be chosen.
I have added the change directly to the GUIDE in NeurodataWithoutBorders/nwb-guide#908 so I suppose this PR is not necessary if you would prefer to close it - just be aware that programmatic usage of NeuroConv without this fix will have the same issue going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks. That gives me more time to think.
Yeah, I believe that the add_to_nwb
and the run_conversion
signatures of the converters should not different for common arguments (in this case, conversion options) so I would rather fix that.
I think we can add helper functions to specify common conversions options for all the interfaces of a converter but I want to push complexity outwards and not to NWBConverter
.
OK, closing as this was fixed at the GUIDE level. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
==========================================
+ Coverage 90.59% 90.60% +0.01%
==========================================
Files 127 127
Lines 7496 7504 +8
==========================================
+ Hits 6791 6799 +8
Misses 705 705
Flags with carried forward coverage won't be shown. Click here to find out more.
|
OK, this should finally finish NeurodataWithoutBorders/nwb-guide#870 @rly
cc @h-mayorquin long-term it might be nice to think about aligning this minor discrepancy between how conversion options are passed between data interfaces and converters