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

[REF] refactor institution and task tables #1397

Merged
merged 19 commits into from Apr 20, 2023

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 5, 2023

Also adds comments for several macros in the nirs page.

@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Patch coverage has no change and project coverage change: +1.82 🎉

Comparison is base (88b4574) 87.88% compared to head (c387331) 89.70%.

❗ Current head c387331 differs from pull request most recent head 0e1f487. Consider uploading reports for the commit 0e1f487 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1397      +/-   ##
==========================================
+ Coverage   87.88%   89.70%   +1.82%     
==========================================
  Files          14       12       -2     
  Lines        1279     1185      -94     
==========================================
- Hits         1124     1063      -61     
+ Misses        155      122      -33     

see 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Remi-Gau Remi-Gau marked this pull request as ready for review February 5, 2023 18:07
@Remi-Gau
Copy link
Collaborator Author

@effigies here too added links and screenshots to point to the "most obvious" changes.

@sappelhoff
Copy link
Member

I find it a bit weird that after the specific sections (that contain REQUIRED, RECOMMENDED, OPTIONAL fields), there are additional sections for REQUIRED, ... fields 🤔 it might lead to confusion, what do you think?

@Remi-Gau
Copy link
Collaborator Author

I find it a bit weird that after the specific sections (that contain REQUIRED, RECOMMENDED, OPTIONAL fields), there are additional sections for REQUIRED, ... fields thinking it might lead to confusion, what do you think?

Maybe a better way to organize things ⬇️ ?

### Sidecar json

- put metadata specific to that datatype but with no headings

- table for required metadata, 
- table for recommended metadata,  
- table for optional metadata 

#### Hardware information

#### Institution information

#### Task information

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 24, 2023

FYI: I am breaking things into separate tables because currently macros do not sort items by their requirement level (AFAIK).

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Mar 6, 2023

I find it a bit weird that after the specific sections (that contain REQUIRED, RECOMMENDED, OPTIONAL fields), there are additional sections for REQUIRED, ... fields thinking it might lead to confusion, what do you think?

Maybe a better way to organize things arrow_down ?

### Sidecar json

- put metadata specific to that datatype but with no headings

- table for required metadata, 
- table for recommended metadata,  
- table for optional metadata 

#### Hardware information

#### Institution information

#### Task information

@sappelhoff I have reorganized things as described above. Let me know what you think.

@effigies @tsalo if you have time for a quick look.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Mar 6, 2023

Actually looking at the new microscopy page, I have question:

Do we agree that, within a metadata table, requirement level takes precedence over alphabetical order for sorting items?

So REQUIRED items appear higher in the tables than RECOMMENDED, but all REQUIRED items must be sorted alphabetically?

If so I should probably reorganize some of the microscopy tables? See the sample one, for example:
https://bids-specification--1397.org.readthedocs.build/en/1397/modality-specific-files/microscopy.html#sample

Alternatively I could also create a table for required items and one for recommended as this a pattern we have in many other datatypes.

Preferences? Suggestions?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I like it now and I think it's an improvement! Thanks Remi

So REQUIRED items appear higher in the tables than RECOMMENDED, but all REQUIRED items must be sorted alphabetically?

+1

@tsalo
Copy link
Member

tsalo commented Mar 6, 2023

Given the scope of the PR I'll need to take a closer look before giving an approving review, but I really like the general change.

@Remi-Gau
Copy link
Collaborator Author

@bids-standard/maintainers if one of you has time to give this another review so it can go in

@rwblair rwblair merged commit 62c37d9 into bids-standard:master Apr 20, 2023
19 checks passed
@Remi-Gau Remi-Gau deleted the tables branch May 23, 2023 00:47
@sappelhoff sappelhoff added the exclude-from-changelog This item will not feature in the automatically generated changelog label Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metadata table refactoring
5 participants