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

Error popups and default X range when no timestamps #27

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

adrien-berchet
Copy link
Contributor

No description provided.

@adrien-berchet adrien-berchet linked an issue Feb 1, 2023 that may be closed by this pull request
2 tasks
Comment on lines +118 to +123
// hdf5::types::TypeDescriptor::Enum(_) => todo!(),
// hdf5::types::TypeDescriptor::Compound(_) => todo!(),
// hdf5::types::TypeDescriptor::FixedArray(_, _) => todo!(),
// hdf5::types::TypeDescriptor::FixedAscii(_) => todo!(),
// hdf5::types::TypeDescriptor::FixedUnicode(_) => todo!(),
// hdf5::types::TypeDescriptor::VarLenArray(_) => todo!(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, we can delete them

Copy link
Contributor

@anilbey anilbey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, with this change the BBP NWBs are supported!
The two-way slider and the separation of the theme section are also very nice.

One question: didn't following the reference to another dataset work?

@anilbey
Copy link
Contributor

anilbey commented Feb 1, 2023

One more feedback: when we resize the popup to be veeery big. Then we cannot resize it back to the small size because the resize icon moves out of the program. Maybe making it non-resizable will be better.

@anilbey
Copy link
Contributor

anilbey commented Feb 2, 2023

One question: didn't following the reference to another dataset work?

I just noticed said something wrong yesterday. Below is the link that Rajnish uses and it works fine. 👍

 link       /data_organization/Cell 1/Step/repetition 1/sweep 13/ics__Step__13 -> /stimulus/presentation/ics__Step__13

@adrien-berchet
Copy link
Contributor Author

adrien-berchet commented Feb 2, 2023

Good catch for the size of the popup, I made it non-resizable :)
I did not try to follow the references yet, I guess we can do it in another PR.

@anilbey
Copy link
Contributor

anilbey commented Feb 2, 2023

I did not try to follow the references yet, I guess we can do it in another PR.

Actually following the references work already. When I use h5dump -n "filename" to display the contents, it shows the links and the NWBView displays what those links are pointing at.

 dataset    /acquisition/ic__Step__1/data
 link       /acquisition/ic__Step__1/electrode -> /general/intracellular_ephys/Electrode 1

The dataset we could not display is of this type (H5T_COMPOUND). I don't know why the type descriptor does not work for this type.

❯ h5dump -d /general/intracellular_ephys/intracellular_recordings/stimuli/stimulus 99111002.nwb      ─╯
HDF5 "99111002.nwb" {
DATASET "/general/intracellular_ephys/intracellular_recordings/stimuli/stimulus" {
   DATATYPE  H5T_COMPOUND {
      H5T_STD_I32LE "idx_start";
      H5T_STD_I32LE "count";
      H5T_REFERENCE { H5T_STD_REF_OBJECT } "timeseries";
   }

@adrien-berchet adrien-berchet merged commit ed6344d into master Feb 2, 2023
@adrien-berchet adrien-berchet deleted the error_popup branch February 2, 2023 08:36
@adrien-berchet
Copy link
Contributor Author

Ah yeah ok, each element of this dataset is a tuple of 3 elements (2 ints and one ref).

@adrien-berchet
Copy link
Contributor Author

adrien-berchet commented Feb 2, 2023

One more weird thing: when I move a plot window of a group that is referenced by another one, there is a weird message appearing in red at the bottom left of the window:

Double use of widget ID 58D6

(or other ID, depending on the window)
I don't know why yet, but it seems it still works properly even with this warning.

EDIT: It happens only when the 2 groups are expanded in the tree.

@anilbey
Copy link
Contributor

anilbey commented Feb 2, 2023

The entity manager of the GUI library stores the Windows using their title names as IDs.
https://github.com/emilk/egui/blob/master/crates/egui/src/containers/window.rs#L38-L40

@adrien-berchet
Copy link
Contributor Author

Hmmm yeah ok, And we set the dataset names as titles, which is probably the same because of the reference. We should probably use the actual dataset path instead of the name to fix this.

@anilbey
Copy link
Contributor

anilbey commented Feb 2, 2023

The whole path can be very long to be displayed on the title though. Maybe the actual dataset path can go to this window.id(...).

/// If you need a changing title, you must call window.id(…) with a fixed id.
https://github.com/emilk/egui/blob/master/crates/egui/src/containers/window.rs#L40

@adrien-berchet
Copy link
Contributor Author

Ah maybe indeed

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.

Add support for BlueBrain's NWBs
2 participants