search

fNIRS / snirf

SNIRF Format Specification
http://fnirs.org/resources/software/snirf/
Other
58 stars 33 forks source link

Added supported datatypes for processed TD moment data #79

Closed sstucker closed 2 years ago

sstucker commented 2 years ago

For time-domain moment data, it would useful to have a few more supported datatype strings codified

dboas commented 2 years ago

@sstucker , I think this has to be raised as an issue for the discussion to converge on the actual labels to use for the new data type labels. Maybe you can link this pull request to that issue. For instance, I don't know how to include Zahra in this pull request discussion. But I think I do know how to include her in the issue discussion.

rob-luke commented 2 years ago

Ping @Zahra-M-Aghajan

rob-luke commented 2 years ago

@sstucker I took the liberty of adding the new terms to the dictionary and adding this as an item in the changelog. I also checked that the table still formats correctly (it does). I'm just learning about TD NIRS now, so I'll leave the discussion to the experts.

sstucker commented 2 years ago

@rob-luke Sorry for not updating the spellcheck dict. Should I have raised an associated issue or is this an acceptable approach to making this change re: our intent to use GitHub release features to freeze the first release?

rob-luke commented 2 years ago

Sorry for not updating the spellcheck dict

No worries, I am happy to help with the admin.

Should I have raised an associated issue or is this an acceptable approach to making this change

Issue as a PR is fine. It's often easier to make more insightful comments on a PR as it contains the detail, and can reduce double handling. The risk is to the submitter, in the case the PR is rejected (unlikely) you've put in more time/work.

re: our intent to use GitHub release features to freeze the first release

Submitting PRs is fine. In the small chance there was a reason to hold this to the next release, we would simply not merge it in to the main branch until it's ready. So this process is fine.

Is there a timeline you need to get this merged in @sstucker? If not, I suggest we give @Zahra-M-Aghajan a few days incase she has a comment.

Zahra-M-Aghajan commented 2 years ago

Thank you for including me in the conversation! I have a comment and a suggestion:

sstucker commented 2 years ago

Comment: Do you think the original dataType 301 (moments data with the dataTypeIndex referring to the moment order) was not sufficient? Is this why you are adding these options to the processed data type as well?

Yes, my understanding is that in Homer3 we would like to be able to specify dataTypeIndex (for HRF index, perhaps) in addition to the datatypeLabel. Can you confirm @dboas?

dboas commented 2 years ago

dataType = 301 is reserved for raw data Time Domain Moments. It is not used for processed data. When we originally specified SNIRF many years ago, all agreed that we should just focus on raw data and we just left processed data for the future. We added dataType = 99999 for processed data. We can think of a most likely set of processed data types, but there will always be other processed data types that people come up with. I think that is why we then added this dataTypeLabel to provide further clarification for dataType = 99999. At least that is how I recall it. No doubt this can be organized better. We should come to consensus for this soon as more and more people will be struggle with this same issue.

For now, I am only struggle with it within Homer where we follow the snirf spec in our data structure. We aren't actually saving our processed data to a snirf file yet since we recalled that snirf developers have not sufficiently discussed how to handle processed data in the snirf spec

rob-luke commented 2 years ago

@sstucker, this is completely opinion so take on board as you wish. But I prefer to use the squash and merge for PRs as it keeps the commit history cleaner. Particularly in projects like this where each PR is just addressing a single issue.

In this example, using squash and merge would add a single item of Added supported datatypes for processed TD moment data to the git history. whereas using a merge commit would result in 6 commits including update changelog, which while relevant, is implied in the single squash commit.

This is absolutely just opinion, and there's a million arguments for each option on the internet, so continue as you were if that's your preference.

image

sstucker commented 2 years ago

I actually intended to squash. I swear it was a default at some point.

On Thu, Sep 23, 2021 at 8:32 PM Robert Luke @.***> wrote:

@sstucker https://github.com/sstucker, this is completely opinion so take on board as you wish. But I prefer to use the squash and merge for PRs as it keeps the commit history cleaner. Particularly in projects like this where each PR is just addressing a single issue.

In this example, using squash and merge would add a single item of Added supported datatypes for processed TD moment data to the git history. whereas using a merge commit would result in 6 commits including update changelog, which while relevant, is implied in the single squash commit.

This is absolutely just opinion, and there's a million arguments for each option on the internet, so continue as you were if that's your preference.

[image: image] https://user-images.githubusercontent.com/748691/134600716-54988c90-c679-49a3-a215-e49ebe9234ee.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fNIRS/snirf/pull/79#issuecomment-926259167, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFKLE5JN3FGFF5XIXBUUNATUDPBKZANCNFSM5ER6N53Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.