FIX: Restore report generation #88
Conversation
effigies
force-pushed
the
fix/reports
branch
from
fbbfb4d to
f165116
Compare
effigies
force-pushed
the
fix/reports
branch
from
7b9f343 to
760830d
Compare
| (l1_metadata, model, [('out', 'stat_metadata')]), | ||
| (model, collate, [('contrast_metadata', 'contrast_metadata')]), | ||
| (stage, model, [('contrast_maps', 'stat_files'), | ||
| ('contrast_metadata', 'stat_metadata')]), |
There was a problem hiding this comment.
@adelavega I'm honestly not sure how l1_metadata worked at the third level, here. It was always supposed to be this simple, and being able to drop the l1_metadata tag (thanks to bids-standard/pybids#319) allows us to do that.
There was a problem hiding this comment.
Hmm. yeah that makes way more sense to me... I'll try it out
fitlins/interfaces/bids.py
Outdated
| names = [col for col in dense.columns | ||
| if col.startswith('non_steady_state') or | ||
| col in step.model['x']] | ||
| names = step.model['x'].copy() |
There was a problem hiding this comment.
This gets the wrong list of names. This includes all dense and sparse columns, so dense[names] fails.
There was a problem hiding this comment.
What was wrong with the previous approach? It basically was all dense columns that were in the model + non_steady_state columns
There was a problem hiding this comment.
Columns came out alphabetized,destroying logical groupings, such as EVs first.
Can update to filter through dense.columns first.
There was a problem hiding this comment.
What logical groupings? You mean order that you put into the model yourself? I'm fine with trying to preserve the order in any case, just confusing what you're referring to.
There was a problem hiding this comment.
Yes, just preserving the model writer's specification. I suspect most people will group related variables together and variables of interest first, so that's all I meant by logical groupings.
|
@adelavega If this works for you, I'll merge. 2+ level reports still aren't ready, as I'll need to figure out another way to reconstruct the contrast matrix for those, but won't get to that before tomorrow. |
This PR has as its goal restoring report generation, and any stylistic/UX fixes that seem appropriate as I work toward that goal.
Currently reports are based on the presence of contrast matrix plots, which were removed in #84, so no reports are generated. Additional concerns:
spec['Model']['X']order.