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

Fix subconverter stubbing #870

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Fix subconverter stubbing #870

merged 11 commits into from
Aug 13, 2024

Conversation

CodyCBakerPhD
Copy link
Collaborator

@rly Please give this a try and let me know

@CodyCBakerPhD CodyCBakerPhD self-assigned this Jun 21, 2024
@CodyCBakerPhD CodyCBakerPhD requested a review from rly June 21, 2024 20:11
@rly
Copy link
Collaborator

rly commented Jun 22, 2024

On the File Metadata page, when I click Next to generate the stub file, I get:

JSONSchemaForm.js:498 Uncaught (in promise) Error: 2 JSON Schema errors detected.
    at JSONSchemaForm.throw (JSONSchemaForm.js:498:15)
    at JSONSchemaForm.validate (JSONSchemaForm.js:602:28)
    at async JSONSchemaForm.validate (JSONSchemaForm.js:565:17)
    at async GuidedFooter.onNext [as __onNext] (GuidedMetadata.js:189:46)

Is there an easy way to show what those errors are?

@rly
Copy link
Collaborator

rly commented Jun 22, 2024

Oh wait. Never mind. I didn't specify subject sex or species.

@CodyCBakerPhD
Copy link
Collaborator Author

They showed up locally as flagged by errors and in red, right?

@rly
Copy link
Collaborator

rly commented Jun 22, 2024

They showed up locally as flagged by errors and in red, right?

Yes. I suggest we improve the error reporting to the user. The red popup just says "Error: 2 JSON Schema errors detected." But that's a separate issue and PR.

The fix here did not work. I looked into why. It looks like when this line executes:
https://github.com/NeurodataWithoutBorders/nwb-guide/blob/main/src/pyflask/manageNeuroconv/manage_neuroconv.py#L923

converter is a CustomNWBConverter, and the available_options value is:

{'required': [], 'properties': {'SpikeGLX Converter': {}}, 'type': 'object', 'additionalProperties': False, '$schema': 'http://json-schema.org/draft-07/schema#', '$id': 'conversion_options.schema.json', 'title': 'Conversion options schema', 'description': 'Schema for the conversion options', 'version': '0.1.0'}

I think available_options.get("properties").get("SpikeGLX Converter") should be a non-empty dictionary of conversion options for each of its subinterfaces (the AP recording interface, LF recording interface, and NIDQ recording interface). I think that would make this new code work. Is that right?

@CodyCBakerPhD
Copy link
Collaborator Author

@rly Fix on NeuroConv was merged: catalystneuro/neuroconv#922

@CodyCBakerPhD CodyCBakerPhD requested review from rly and removed request for rly June 25, 2024 17:05
@CodyCBakerPhD CodyCBakerPhD enabled auto-merge June 25, 2024 17:05
@CodyCBakerPhD
Copy link
Collaborator Author

ping @rly

@rly
Copy link
Collaborator

rly commented Jun 26, 2024

The preview is still converting the whole dataset. I will poke around and see what I can figure out.

@CodyCBakerPhD
Copy link
Collaborator Author

Huh, thought you said it was working with the NeuroConv dev branch?

@rly
Copy link
Collaborator

rly commented Jun 26, 2024

That's so strange. I just tried again with the commit from catalystneuro/neuroconv#922 right before I commented, and it converted the whole dataset. I must not have checked thoroughly or done something odd when testing that neuroconv PR. Sorry about that.

CustomNWBConverter contains a SpikeGLXConverter which contains two SpikeGLXRecordingInterface and a SpikeGLXNIDQInterface. With the latest change, SpikeGLXConverter has "properties" so the new if statement on line 931 is not entered (code below). When I try to address that by checking whether the converter is a NWBConverter instead, I get an error add_to_nwbfile() got an unexpected keyword argument 'imec0.lf'.

I think https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/nwbconverter.py#L158 is not set up to work with an NWBConverter that contains a NWBConverter because it expects an argument named "conversion_options" and when the call is made recursively, there is no argument named "conversion_options" being passed. If I try to package the options within a "conversion_options", I get a jsonschema.exceptions.ValidationError: Additional properties are not allowed ('conversion_options' was unexpected).

After going down this rabbit hole, I am not sure what the best approach is. What do you think? Does this need to be addressed on the neuroconv side or here?

        progress_bar_options = dict(
            mininterval=0,
            on_progress_update=update_conversion_progress,
        )

        def update_conversion_options(converter_local, available_options_local, options_local):
            if isinstance(converter_local, neuroconv.NWBConverter):
                print(f"NWBConverter {converter_local} entered")
                for name, subconverter in converter_local.data_interface_objects.items():
                    options_local[name] = {"conversion_options": {}}
                    print(f"{name} interface entered")
                    print(f"available_options_local: {available_options_local}")
                    print(f"options_local: {options_local}")
                    update_conversion_options(
                        subconverter,
                        available_options_local.get("properties").get(name),
                        options_local[name]["conversion_options"]
                    )
            else:
                print(f"{converter_local} interface entered")
                available_opts = available_options_local.get("properties", {})

                if run_stub_test and "stub_test" in available_opts:
                    options_local["stub_test"] = True
                    print(f"{converter_local} has been stubbed")

                # if "iterator_opts" in available_opts:
                #     options_local["iterator_opts"] = dict(
                #         display_progress=True,
                #         progress_bar_class=TQDMProgressSubscriber,
                #         progress_bar_options=progress_bar_options,
                #     )

        # Assume all interfaces have the same conversion options for now
        available_options = converter.get_conversion_options_schema()
        options = {interface: {} for interface in info["source_data"]}

        update_conversion_options(converter, available_options, options)

        # Add GUIDE watermark
        package_json_file_path = resource_path("package.json" if is_packaged() else "../package.json")

        # ...

@CodyCBakerPhD
Copy link
Collaborator Author

I'll take another look next week, busy the next couple of days

@CodyCBakerPhD
Copy link
Collaborator Author

This works with catalystneuro/neuroconv#979 now

I will ping you again on this after NeuroConv PR is merged @rly

@CodyCBakerPhD CodyCBakerPhD marked this pull request as draft August 5, 2024 18:27
auto-merge was automatically disabled August 5, 2024 18:27

Pull request was converted to draft

@CodyCBakerPhD CodyCBakerPhD merged commit 086cb16 into main Aug 13, 2024
5 of 8 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the fix_subconverter_stubbing branch August 13, 2024 23:42
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.

2 participants