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

[ENH] Add session ID as a new category for annotation #675

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Jan 15, 2024

Changes proposed in this pull request:

  • add Session ID as a category
  • add new color for session id
  • make sure session ID does not count as an annotation and does not get its own annotation workflow (like Subject ID)
  • add a simple e2e test to make sure of this

Note: this should probably have more extensive tests, I went for a basic test because we may soon refactor the app. But could add appropriate tests as well.

Checklist

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see https://neurobagel.org/contributing/pull_requests for more info)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Copy link

netlify bot commented Jan 15, 2024

Deploy Preview for neurobagel-annotator ready!

Name Link
🔨 Latest commit d35d886
🔍 Latest deploy log https://app.netlify.com/sites/neurobagel-annotator/deploys/65a5a17329e63400086bc6c6
😎 Deploy Preview https://deploy-preview-675--neurobagel-annotator.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@surchs surchs added the pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0) label Jan 15, 2024
@rmanaem rmanaem self-requested a review January 17, 2024 15:35
Copy link
Contributor

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

Everything looks good 🧑‍🍳
It's a good idea to update the test containing the output validation using schema to ensure the output is valid after the session ID is annotated, wdyt?

@surchs
Copy link
Contributor Author

surchs commented Jan 17, 2024

good idea to update the test containing the output validation using schema to ensure the output is valid after the session ID is annotated

Yeah, I was thinking about that. The main thing is that our current example files don't have a session_id column and I didn't want to change the example / introduce a new one. I could just have it label an incorrect column as session_id and see what happens. Or do you have a better idea?

@rmanaem
Copy link
Contributor

rmanaem commented Jan 17, 2024

We could make an issue and address it later. The issue could include more than just updating the test e.g., adding a new example(s) that cover all columns and variables.

@surchs surchs merged commit 85329cf into main Jan 17, 2024
8 checks passed
@surchs surchs deleted the issue663 branch January 17, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0)
Projects
None yet
2 participants