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

Ophys Metadata #505

Merged
merged 183 commits into from
Jan 27, 2024
Merged

Ophys Metadata #505

merged 183 commits into from
Jan 27, 2024

Conversation

garrettmflynn
Copy link
Member

This PR enables the Ophys Metadata and allow for editing table properties correctly to update the File Metadata structure passed to the server.

Ongoing work on this PR will include the following updates:

  1. Inter-table updates for valid device / imaging plane selection.
  2. Updating NeuroConv-internal representations (similar to Ecephys)
  3. Ensure we actually want to represent all of the metadata
    • As per a recent discussion, the PlaneSegmentationTable could have tens of thousands of rows (ROIs). We could render a subset of the table and otherwise allow for CSV import / export using BasicTable.

@garrettmflynn garrettmflynn self-assigned this Nov 9, 2023
@CodyCBakerPhD
Copy link
Collaborator

This is a great first draft!

Starting with ImagingInterface metadata

  • disable manifold, unit, and conversion in ImagingPlane section (in general, drop anything whose description includes the phrase 'DEPRECATED: ...')
  • drop format, starting_frame, starting_time+rate (will be reintroduced in temporal alignment feature; also don't want user overriding known rate), bits_per_pixel, dimension (this can be set automatically, don't want user to provide something that conflicts), control, control_description, and device (confused why this one is even in the schema for the 2P series lol - it's in the ImagingPlane link after all...) from TwoPhotonSeries section
  • reorgnize the TwoPhotonSeries columns to interrelate a bit more; suggestion, name -> description (one caveat - sometimes this is a big JSON dictionary with acquisition settings so it doesn't render super well - see the ScanImage testing data for an example) -> scan_line_rate -> field_of_view -> unit (see next comment) -> conversion -> offset
  • One idea would be to ask if the user knows the unit of the 2P data (yes or no), which is quite uncommon for raw microscopy. If they say 'no' then drop unit (set to n.a. auomatically) as well as conversion and offset since they have no meaning in the metadata any more
  • reorganize the ImagingPlane columns similarly, at least starting with name -> description -> Device -> OpticChannel

I love the nested table for OpticChannel BTW, we should think of ways to utilize that in other metadata places

Base automatically changed from disable-schema-props to main November 10, 2023 20:27
@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Jan 25, 2024

Also on the current auto-generated ScanImage pipeline, I get

base-metadata.schema.ts:95 Uncaught (in promise) TypeError: Cannot set properties of undefined (setting 'order')
    at preprocessMetadataSchema (base-metadata.schema.ts:95:27)
    at createForm (GuidedMetadata.js:187:32)
    at utils.js:69:73
    at Array.map (<anonymous>)
    at utils.js:69:45
    at Array.map (<anonymous>)
    at mapSessions (utils.js:68:10)
    at GuidedMetadataPage.mapSessions (Page.js:117:63)
    at GuidedMetadataPage.render (GuidedMetadata.js:444:27)
    at GuidedMetadataPage.update (lit-element.ts:160:24)

on the file metadata page

@CodyCBakerPhD
Copy link
Collaborator

Very weird issue possible related. Steps to reproduce

  1. autogenerate fresh testing pipelines
  2. start with ScanImage, go to the file metadata page if you can (+/- the error above if you see it, either way proceed to 3)
  3. exit out and go to suite2p or caiman
  4. proceed to file metadata page
  5. exit/save and go back to scanimage pipeline
  6. I see the suite2p/caiman metadata page on the scanimage pipeline now

@garrettmflynn
Copy link
Member Author

Also on the current auto-generated ScanImage pipeline, I get

base-metadata.schema.ts:95 Uncaught (in promise) TypeError: Cannot set properties of undefined (setting 'order')
    at preprocessMetadataSchema (base-metadata.schema.ts:95:27)
    at createForm (GuidedMetadata.js:187:32)
    at utils.js:69:73
    at Array.map (<anonymous>)
    at utils.js:69:45
    at Array.map (<anonymous>)
    at mapSessions (utils.js:68:10)
    at GuidedMetadataPage.mapSessions (Page.js:117:63)
    at GuidedMetadataPage.render (GuidedMetadata.js:444:27)
    at GuidedMetadataPage.update (lit-element.ts:160:24)

on the file metadata page

Oddly, this example has the items property defined as undefined on the ImagingPlane or TwoPhotonSeries schema. Should this be passed like this from NeuroConv?

@garrettmflynn
Copy link
Member Author

Hmm this appears to be consistent now, though I'd never seen it before.

@garrettmflynn
Copy link
Member Author

Oh looks like the definitions resolution is not happening correctly anymore. One sec

@garrettmflynn
Copy link
Member Author

Odd behavior observed in any segmentation pipeline (this PR can now autogenerate suite2p and caiman for easy testing): go to the Fluorescence level and change resolution from -1 to any other number - I then get informed that it must be a float not an int, which I then correct to be a float, but still get the error on attempting to generate preview - then trying to restore it to the default of -1 continues to cause the error beyond that

So the core problem here is that JavaScript does not have the ability to maintain floats that are just integers (i.e. -1.0 is equivalent to -1, and will be sent to Python as such).

When you haven't manually changed the value, there will be nothing sent to Python for that property—allowing the default to take effect. However, manually providing -1 (or -1.0) will result in a -1 integer registered in Python.

We will probably have to coerce to a float similar to our previous NaN parsing on the Python end.

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jan 25, 2024

Though I notice I'm actually getting this error now even when I don't modify the resolution field at all; seems that manually modifying any field on the table will trigger it - are you able to reproduce?

I can reproduce this additional behavior though. Likely the displayed default value is being registered as a real value—which is an oversight of mine for these deeply nested tables (as their changes are submitted as the whole table, not granular changes).

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jan 25, 2024

Can you check if this still happens? #505 (comment)

I wasn't able to get this to replicate now. Originally, I wasn't able to get anything to render on ScanImage because of the missing items, so that might be why it didn't update the previous render.

@CodyCBakerPhD
Copy link
Collaborator

Can you check if this still happens? #505 (comment)

Ah, apparently not

So the core problem here is that JavaScript does not have the ability to maintain floats that are just integers (i.e. -1.0 is equivalent to -1, and will be sent to Python as such).

When you haven't manually changed the value, there will be nothing sent to Python for that property—allowing the default to take effect. However, manually providing -1 (or -1.0) will result in a -1 integer registered in Python.

We will probably have to coerce to a float similar to our previous NaN parsing on the Python end.

Right, that all makes sense. Casting would be fine (the default should also be -1.0 in that case anyway); checking the NeuroConv hardcoded schemas though, I don't actually see exposure of the resolution argument (again, one of those I don't think majority of people would ever willingly specify, or have even indicated knowing except in special cases) so if it's easier for you at the moment I say we just suppress it from all the tables

@CodyCBakerPhD
Copy link
Collaborator

I found a couple unrelated bugs while thoroughly testing this; they can be solved in follow-ups

I think this is just about ready to merge once the resolution thing is dealt with

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review January 26, 2024 04:01
@garrettmflynn
Copy link
Member Author

Figured out the root of the resolution issue. I have been coercing to floats for some time now, though I wasn't searching through the patternProperties for values to coerce. This is now implemented!

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) January 27, 2024 18:13
@CodyCBakerPhD CodyCBakerPhD merged commit 18d20f3 into main Jan 27, 2024
10 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the ophys-metadata branch January 27, 2024 18:17
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