cms-analysis / HiggsAnalysis-CombinedLimit

CMS Higgs Combination toolkit.
https://cms-analysis.github.io/HiggsAnalysis-CombinedLimit/latest
Apache License 2.0
75 stars 381 forks source link

Nckw add example teststat plots #845

Closed nucleosynthesis closed 1 year ago

nucleosynthesis commented 1 year ago

Add example plots of test-stat distributions in part explaining limit calculation

nucleosynthesis commented 1 year ago

@kcormi - latest commit is an attempt to address your comments. Let me know what you think

nucleosynthesis commented 1 year ago

Looks like I spoke too soon, the figures seem to appear inside the show/hide box in the preview on GitHub but don't seem to work on the docs page - any ideas?

https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/docs/part3/commonstatsmethods.md?plain=1#L607-L615

adewit commented 1 year ago

Did you try a relative path (and maybe even the include syntax used elsewhere?), e.g.

![](images/exampleLHC.jpg)

I have a vague recollection that relative/absolute paths caused some problems at least with page links, so maybe worth a try?

kcormi commented 1 year ago

It could be an issue with using the absolute path, I had issues with the paths being treated slightly differently in a local test and in a deployment previously.

When I inspect other images on that page in the browser, they are given by relative paths "../images/.png", whereas, these ones are using the absolute path with "/docs" as the root.

nucleosynthesis commented 1 year ago

Yeah for a standard image, the relative path is correct but this doesn't seem to work inside a <details> block (where it seems we have to use the html <img ... > thing). I tried a few different versions and none of them seemed to work so what I have now (which does work at least) is the full raw path to the image. Unfortunately, it seems hard to test since the preview in GitHub doesn't show the behaviour of the html that gets built.

kcormi commented 1 year ago

I set up a similar github.io page on my own github account for testing some of the other documentation updates. I can try using that one to make some tests and see what works.

kcormi commented 1 year ago

Also, if its related to the \<details> block, I wonder if it is handled properly with some of the updates that will come in as part of #839. There I added the pymdownx.blocks.details markdown extension, to make use of the

/// details | header name

contents
///

block style. I wonder if markdown then handles the image stuff properly. I can also check when I run a few tests shortly.

nucleosynthesis commented 1 year ago

Right, if you can check whether using the pymdownx extension works, i'd rather swap to using that here too. There's probably a few other places where we might want to hide figures for ease of reading but if we get this change in, that will make it easier to do. Thanks!

On Thu, Jul 6, 2023 at 10:37 AM kcormi @.***> wrote:

Also, if its related to the block, I wonder if it is handled properly with some of the updates that will come in as part of #839 https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/pull/839. There I added the pymdownx.blocks.details markdown extension, to make use of the

/// details | header name

contents ///

block style. I wonder if markdown then handles the image stuff properly. I can also check when I run a few tests shortly.

— Reply to this email directly, view it on GitHub https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/pull/845#issuecomment-1623346949, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMEVW5ETZVYG7MW3I2TTITXO2BL5ANCNFSM6AAAAAAZZ3SYDA . You are receiving this because you modified the open/close state.Message ID: @.*** com>

kcormi commented 1 year ago

It seems to work using the details block extension:

https://kcormi.github.io/HiggsAnalysis-CombinedLimit/part3/commonstatsmethods/#complex-models

code is here: https://github.com/kcormi/HiggsAnalysis-CombinedLimit/blob/docs_test_deploy/docs/part3/commonstatsmethods.md?plain=1#L607

It seems also possible to do it without the extension, but I also had to set the width explicitly in the img html block, otherwise it displayed very poorly. The code: https://github.com/kcormi/HiggsAnalysis-CombinedLimit/commit/899a2af324b83c62590aa272f4674220a2af19dd#diff-e1cf5d8257ea3082032f066028c25c4e5134426b497d6f3f40042e955177759dR607

Also, for testing purposes, the behaviour I got using a local mkdocs build and just checking it in my browser seemed to reproduce the same results as the actual deployed page. I guess this makes sense that it is slightly different behaviour than what the github preview gives, both using markdown, but I guess with slightly different details.

Instructions on setting up a local environment for testing the documentation is in the contributing guide: https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/blob/main/contributing.md#technical-details-of-the-documentation

kcormi commented 1 year ago

Actually, the local mkdocs build also works with absolute paths, even though it doesn't on deployment. So that is one issue the local test won't catch. Hopefully its the only one, I'll add a note about that in the contributing guide.

nucleosynthesis commented 1 year ago

Thanks Kyle - can we just add the extension to the main branch and use your version of the code?

On Thu, Jul 6, 2023 at 11:20 AM kcormi @.***> wrote:

Actually, the local mkdocs build also works with absolute paths, even though it doesn't on deployment. So that is one issue the local test won't catch. Hopefully its the only one, I'll add a note about that in the contributing guide.

— Reply to this email directly, view it on GitHub https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/pull/845#issuecomment-1623417184, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMEVW3FQRBWO62HHXLWUGTXO2GP7ANCNFSM6AAAAAAZZ3SYDA . You are receiving this because you modified the open/close state.Message ID: @.*** com>