Skip to content
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

New plotting functions for MHOU #1832

Closed
wants to merge 10 commits into from
Closed

New plotting functions for MHOU #1832

wants to merge 10 commits into from

Conversation

andreab1997
Copy link
Contributor

Given that we will need several new kind of plots for the MHOU and N3LO papers, I suggest we collect here the new functions. @giacomomagni

CC: @scarlehoff

@andreab1997 andreab1997 marked this pull request as draft October 31, 2023 13:38
@giacomomagni
Copy link
Contributor

@andreab1997 if you can (and know how to do it), would you mind replacing the label: "NNLO-NLO Shift" in validphys2/src/validphys/theorycovariance/test.py with somethingmore generic ?

@andreab1997
Copy link
Contributor Author

@andreab1997 if you can (and know how to do it), would you mind replacing the label: "NNLO-NLO Shift" in validphys2/src/validphys/theorycovariance/test.py with somethingmore generic ?

So of course something more generic is easy to have. Would you like something like "Shift"?

@giacomomagni
Copy link
Contributor

So of course something more generic is easy to have. Would you like something like "Shift"?

it would be great to have order1 - order2 Shift but not sure how difficult is. So I'm okay with anything.

@andreab1997
Copy link
Contributor Author

So of course something more generic is easy to have. Would you like something like "Shift"?

it would be great to have order1 - order2 Shift but not sure how difficult is. So I'm okay with anything.

Yes indeed, this is what I am going to try first

@scarlehoff
Copy link
Member

It would be great to remove the unused code in the theorycovariance module before adding more...

@andreab1997
Copy link
Contributor Author

It would be great to remove the unused code in the theorycovariance module before adding more...

I agree but this is here just to avoid rewriting code a lot of times. Once we will be sure about which plots we want to have in the papers I will close this and open another in which I will polish a bit the theorycovariance module and add all and only the relevant functions.

@scarlehoff scarlehoff mentioned this pull request Nov 17, 2023
34 tasks
@andreab1997
Copy link
Contributor Author

@scarlehoff I believe the theorycovariance module is cleaned now (there might be a few other lines of code that could be removed but most of the work is done). I would also like to address #1847, would you like me to do that here? Or, should I open another PR?

Also, if you have other suggestions regarding the theorycovariance module, please write them here (or in an issue).

@scarlehoff
Copy link
Member

would you like me to do that here? Or, should I open another PR?

I would say open a separate PR (but taking this one as the initial state) that way if something goes wrong it is easier to revert.

@scarlehoff
Copy link
Member

Is this ready for review?

@andreab1997
Copy link
Contributor Author

Is this ready for review?

Regarding the cleaning yes, but it is not yet ready to be merged as I want at least to address #1847. If in the meanwhile you want to have a look, you are more than welcome

@scarlehoff
Copy link
Member

If #1847 is the only thing missing it would be better in a separate PR (that starts from this one)

@RoyStegeman
Copy link
Member

RoyStegeman commented Dec 15, 2023

@achiefa and I noticed that combine_by_type is doing some weird things because the inputs are not ordered in the same way. each_dataset_results_central_bytheory is ordered by process while dataset_names follows the runcard order which leads to mistakes in process_info. I understand from @andreab1997 that this is not a problem since when running n3fit the covmat it is reordered in agreement with the experimental covmat before it's used, but it's certainly very confusing and worth correcting.

Having a closer look I think @andreab1997 meant the reindexing in this line, which (I assume) ensures that the indices of theory_covmat_custom are ordered in the same way as the experimental covmat:

df = theory_covmat_custom.reindex(procs_index_matched).T.reindex(procs_index_matched)

However, for this to work as intended the indices of theory_covmat_custom have to be correct int he first place, and it's difficult to understand if this is indeed the case since datasets and processes are not always correctly matched in combine_by_type. I.e. it's not just a matter of reindiexing, which makes things a bit difficult to read but I also don't see an obvious way around this, but there's a genuine bug in that function (that for now I'll assume does somehow not affect the final result of a MHOU fit).

@andreab1997
Copy link
Contributor Author

Given that this is now outdated and that the pull plots are now in #1910, I am closing this.

@andreab1997 andreab1997 deleted the pull_plots branch January 16, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants