NNPDF / reportengine

A framework for declarative data analysis
https://data.nnpdf.science/validphys-docs/guide.html
GNU General Public License v2.0
1 stars 2 forks source link

Split up plots into columns in vp reports #33

Closed voisey closed 4 years ago

voisey commented 4 years ago

Works towards closing https://github.com/NNPDF/nnpdf/issues/268. So far it works for the 'PDF plots' section of the vp-comparefits report (https://vp.nnpdf.science/KbFpz1hOQwKsPbFVaDtqew==/pdf_report_report.html), i.e. for the PDF plots and the effective exponents. Personally I think it's much more readable with this formatting and the PDF plots page being shorter is certainly nicer. I think the same formatting should be applied to the other plots in the report too: namely distances, arc lengths, training-validation splits, and positivity plots. Let me know if you have any thoughts

voisey commented 4 years ago

The reason only certain plots have been affected is because some are tagged with @figuregen while some are tagged with @figure. @Zaharid, would it be sensible to edit savefig so that the figures tagged with @figure are put in columns too?

voisey commented 4 years ago

@scarrazza @Zaharid I wasn't totally clear on the outcome of yesterday's discussion on this. Is this ready to be reviewed/merged or is there something else you'd like to be done?

scarrazza commented 4 years ago

@voisey, fine by me from a conceptual point of view.

Zaharid commented 4 years ago

I think we should merge this more or less as is.

A few more general points:

Zaharid commented 4 years ago

It would probably better to add a class instead of a style tag to these reports. And then set the set the style for the class in the css file.

Zaharid commented 4 years ago

@voisey probably we could look into this https://primer.style/css/objects/grid#inline-block-grids we are already using this css style so adding the right classes should do the trick.

voisey commented 4 years ago

On your last point @Zaharid, do you mean that we can ditch the float business altogether and use d-inline-block instead? I don't really understand how this would work practically. Should something like:

<p class="d-inline-block col-2 p-2">
    <img class="width-full avatar" src="..." alt="..." />
</p>

work in our case?

voisey commented 4 years ago

Either way, I've put the styling I used in a CSS class

voisey commented 4 years ago

Regarding your first point in https://github.com/NNPDF/reportengine/pull/33#issuecomment-618404402, where did this file come from?

Zaharid commented 4 years ago

@voisey there is this https://github.com/NNPDF/reportengine/tree/master/src/reportengine/styles the CSS style is some old version of Primer CSS that @scarrazza got. The template is a pandoc template minimally adapted to work with the style.

voisey commented 4 years ago

Can we merge this and then open an issue about updating the styles?

Zaharid commented 4 years ago

Ok I'm going to stop trying to "improve" this. It is already an improvement. Thanks!

voisey commented 4 years ago

Haha, sure. You're welcome!