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
[MRG, API] Start of automatic methods section with create_methods_paragraph #457
Conversation
nice, can you share an example paragraph generated by this report? |
maybe update the ds000117 example? |
Done! lmk what you think.
It is copied into the PR description. |
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
==========================================
- Coverage 93.70% 92.66% -1.04%
==========================================
Files 11 13 +2
Lines 1762 1950 +188
==========================================
+ Hits 1651 1807 +156
- Misses 111 143 +32
Continue to review full report at Codecov.
|
I don't get this part. Can you make the text almost like a copy-paste for publication? I would also add information about:
what's the output of |
This is the output for ds000117:
This seems off. There aren't "6 datasets". We should test on a few datasets to ensure it gives something reasonable. I would pick 10 ephys datasets at random from openneuro and try on them. |
What would be the copy-paste version of this part? Do you have a specific structure in mind?
By filtering info, I suppose you mean the
|
There are 6 |
@adam2392 please paste here what you obtain on various datasets to get a feeling of how it reads. Thx |
I think they are 6 runs not dataset. To me, a dataset is everything inside bids_root |
A few thoughts:
|
@adam2392 can you put this comment in the PR description? |
@jasmainak Besides the current breaking of the bids-validator, I was wondering if you could lmk how the direction currently feels. Some notes:
|
not sure I understand this point. But I would say, don't add too much code complexity and branching. It will make life harder for future developers. can you add a couple of more examples in the description? At least 6 or 7 in total? Just to get a sense of how the methods paragraph might be useful/handy for researchers. I'll ask a couple of my colleagues to provide feedback what else might be useful to include. |
We don't really know if it was MNE-BIDS? If so, we should leave it out |
Is this still true in the context of #460 ? |
@adam2392 take a look at the datasets tested in MNE-study-template. Would you mind posting the description for these datasets as well? If we have a substantial number (around 10), we can start to see if this looks good. And then in the study template, you could add a line to add the generated paragraph to the MNE report using |
Okay, so I took those datasets and running those locally and pasting into the PR description. Also, FYI some of the descriptions aren't up to date w/ some of the minor changes, but I don't want to re-download the datasets locally and redo (They're mainly spelling or grammar issues, or logic on the template string itself, which have been updated). If this is absolutely needed, I can re-download, but it takes a bit of time on my currently home internet :p. I don't now what you mean by adding it into the study template? Do you mean in the
|
@adam2392 please paste here examples of results so we don't all need to test ourself to give you more feedback. Any dataset is fine but the more the better. we just need to read some examples. |
Not sure why the docs are failing... Do you know if it's something I can fix? |
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
I like what Richard suggests
|
One issue I'm having is... is there a way one can "add" |
please go ahead and incorporate @hoechenberger and @sappelhoff 's suggestions :-) I am lagging behind ... |
Okay I added in the feedback and I think things are all good to go. |
here is what I get
|
Looks pretty fair! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 on merging this!
We can keep tweaking more in future PRs but this is good for a start. |
ok for you @hoechenberger ? |
Any remaining changes needed here? Don't want this to go stale and we forget what happened :p. |
I can't make enough time to review this right now, sorry :| |
maybe we merge this and improve later?
|
+1, I hate stale PRs and love iterations :-) |
Merged, thanks a ton @adam2392 !!! This is great :-) |
Argh I was just working on a review… but yet let's iterate, then! :) |
oops, sorry :)
…On Thu, Jul 30, 2020 at 5:33 PM Richard Höchenberger < ***@***.***> wrote:
Argh I was just working on a review… but yet let's iterate, then! :)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#457 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADY6FIVRSFJCZI2J3PRVAQ3R6HRKJANCNFSM4OE6BDQQ>
.
|
PR Description
Addresses preliminarily: #347
A summary of MEEG summary (MEG, EEG, iEEG):
I don't want this to be too much headache to start with, so figured that the easiest most robust summary we can provide is modularized as such:
participants.tsv
file summary per subject: age, sex, hand that is supported in mne-bids. Note this file is only RECOMMENDED. I went through a lot of effort to get this to work without making the report look crazy ugly for now because this I figured is one of the most crucial summaries every study should have, but since it's structure is not very imposed by BIDS, then it's hard to summarize consistently.modality-specific-summary: adding iEEG channel counts (e.g. SEEG, ECoG, etc.). Similarly, I suppose if someone wants to add MEG/EEG, it can be a relatively short summary here.(tabled to future)TODO:
convert_group_studies
to usecreate_methods_paragraph
.kind=ieeg
data (idk how to add for 'eeg', or 'meg', so would prefer someone else add that functionality in)dataset_description.json
and the reference/DOI for mne-bids.emptyroom
subjects? I don't work w/ these, so skipping them for now. Assuming this is okay, I added aXXX
to the inline comments for someone else to fix.*_scans.tsv
files, which is considered "RECOMMENDED" and not "REQUIRED".Example Output from Local/OpenNeuro datasets
These are the datasets I ran the method generation w/:
Ran datasets w/ the following code:
See output on PR here:
./report.txt
.Merge checklist
Maintainer, please confirm the following before merging: