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.

Sign up for GitHub

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

Jump to bottom

[EEG Browser] Annotations: Add new and HED tags #8236

Merged
merged 6 commits into from
Dec 1, 2022
Merged

[EEG Browser] Annotations: Add new and HED tags #8236

merged 6 commits into from
Dec 1, 2022

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Nov 22, 2022 •
edited by christinerogers
Loading

Replaces #7829 including custom code from the EEGNet repo to date

@laemtl laemtl added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 22, 2022
@laemtl laemtl force-pushed the 2021_11_12_support_for_adding_new_annotations_from_EEG_browser branch from 8d8fc51 to 1ff23f4 Compare November 22, 2022 20:37
@laemtl laemtl removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 24, 2022
@laemtl laemtl force-pushed the 2021_11_12_support_for_adding_new_annotations_from_EEG_browser branch from 1ff23f4 to 5063dda Compare November 25, 2022 01:24
@laemtl laemtl force-pushed the 2021_11_12_support_for_adding_new_annotations_from_EEG_browser branch 2 times, most recently from 923efdd to 5130d0c Compare November 25, 2022 03:52
@laemtl laemtl changed the title [EEG Browser][Feature] Ability to add new annotations from module [EEG Browser] Add new annotations and HED tags Nov 25, 2022
@laemtl laemtl force-pushed the 2021_11_12_support_for_adding_new_annotations_from_EEG_browser branch 3 times, most recently from 43ac725 to cbf5d67 Compare November 25, 2022 04:12
@laemtl laemtl changed the title [EEG Browser] Add new annotations and HED tags [EEG Browser] Annotations: Add new and HED tags Nov 25, 2022
@laemtl laemtl force-pushed the 2021_11_12_support_for_adding_new_annotations_from_EEG_browser branch from cbf5d67 to 9f75ebe Compare November 25, 2022 04:54
@laemtl laemtl requested a review from maltheism November 25, 2022 14:58
maltheism
maltheism reviewed Nov 25, 2022
View reviewed changes
modules/electrophysiology_browser/php/annotations.class.inc Outdated
$db = $factory->database();
$factory = \NDB_Factory::singleton();
$user = $factory->user();
$db = $factory->database();
Copy link
Member

Choose a reason for hiding this comment

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

Hi @laemtl you could do the following instead of initializing the factory.

$user = $request->getAttribute('user');
$db    = $request->getAttribute("loris")->getDatabaseConnection();

maltheism
maltheism requested changes Nov 25, 2022
View reviewed changes
Copy link
Member

@maltheism maltheism left a comment

Choose a reason for hiding this comment

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

Hi @laemtl the frontend code should have comments that will satisfy the "valid-jsdoc of eslint" even if disabled. Otherwise everything looks okay although I'm unable to manually test.

@laemtl
Copy link
Contributor Author

laemtl commented Nov 28, 2022 •
edited
Loading

Hi @laemtl the frontend code should have comments that will satisfy the "valid-jsdoc of eslint" even if disabled. Otherwise everything looks okay although I'm unable to manually test.

I am working on a separate PR that will enable ESLint and fix all those issues. It would be easier since I have one PR based on that one that I would like to rebase.

@laemtl laemtl requested a review from maltheism November 28, 2022 16:55
@laemtl laemtl added the Blocking PR should be prioritized because it is blocking the progress of another task label Nov 28, 2022
@driusan driusan merged commit f427144 into aces:main Dec 1, 2022
@christinerogers
Copy link
Contributor

This PR replaces #7828 and #7829 original PRs contributed Nov.2021 by @jesscall for EEGNet.
Additional EEGNet custom code was also wrapped into this PR (work by @jesscall and @Andesha)
Thanks @laemtl for bringing this forward and getting the convergence resolved quickly.

@christinerogers christinerogers added Release: Add to release notes PR whose changes should be highlighted in the release notes and removed Blocking PR should be prioritized because it is blocking the progress of another task labels Feb 9, 2023
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maltheism maltheism maltheism approved these changes

Assignees
No one assigned
Labels
Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@laemtl @christinerogers @maltheism @driusan @ridz1208