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

[electrophysiology_browser] Replace Physiological Annotations with Events #9032

Merged

Conversation

jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Jan 31, 2024

This PR aims to improve the current compliance to the BIDS standard. Loris-MRI counterpart: #1041
Partially addresses #8910.

This replaces references to annotation files with references to event files.

This PR deletes *_annotation_* tables. The corresponding required event tables already exist.

+minor improvements to the EventManager and AnnotationForm components.

This PR brings in the following limitations:

  • Events can no longer be created via the interface (support coming soon)
  • Events can be modified, but the currently supported fields are by default read-only or disabled (Name and time)
  • HED tag support is extremely limited (better support in imminent PR: [electrophysiology_browser] HED Tag Support #9033)

TODO: Raisinbread modifications

TODO: Get official server-side solution + Add to documentation.

  • Determine required request time limit
  • Determine required memory allocation limit
  • Determine procedure for increasing when needed

Both of the above needed to be increased for certain requests.

jeffersoncasimir added a commit to jeffersoncasimir/Loris that referenced this pull request Feb 1, 2024
@christinerogers christinerogers added Release: Add to release notes PR whose changes should be highlighted in the release notes Cleanup PR or issue introducing/requiring at least one clean-up operation Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing labels Feb 6, 2024
@driusan
Copy link
Collaborator

driusan commented Feb 6, 2024

@regisoc can you review this?

Copy link
Contributor

@regisoc regisoc left a comment

Choose a reason for hiding this comment

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

Nice work!

  • global logic looks good. Some questions for the back-end class.
  • Still some TODOs in the code. I avoided repeating all comments. The comment on the first one should be valid for all similar following it.
  • rename permission electrophysiology_browser_edit_annotations to electrophysiology_browser_edit_events?
  • Some cleaning to do (commented lines to remove/keep)?
  • Not tested yet. I might need help setting up testing env.

@regisoc
Copy link
Contributor

regisoc commented Feb 14, 2024

I will put that here so we do not forget them. Note after tests:

  • Event download button disabled or not working.
  • Error epoch.properties is undefined following that process:
    • open event panel
    • edit one event
    • submit this event
    • close the event submit with the top-right "x".

@regisoc
Copy link
Contributor

regisoc commented Feb 14, 2024

@jeffersoncasimir let me know if you need another round of test this morning.

Copy link
Contributor

@regisoc regisoc left a comment

Choose a reason for hiding this comment

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

New pass:

  • error on epoch resolved.
  • new errors due to multiple config db calls that can vary from config.xml. Unify calls for config check to $config->getSetting("dataDirBasepath"); in electrophysiologyevents.class.inc (one in _updateArchives one in update).
  • Archive events file tgz created and downloadable, but archive is corrupted. Not corrupted, but not a gzip archive? Using tar xf <file> works, but not when using tar xzf <file>?

Copy link
Contributor

@regisoc regisoc left a comment

Choose a reason for hiding this comment

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

LGTM

@christinerogers christinerogers added this to the 26.0.0 milestone Feb 23, 2024
@laemtl laemtl assigned driusan and unassigned regisoc Feb 27, 2024
@driusan driusan merged commit 1112e1a into aces:main Feb 27, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants