-
Notifications
You must be signed in to change notification settings - Fork 7
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
Extend th covmat plots #1910
Extend th covmat plots #1910
Conversation
@giacomomagni I would say that in this PR we could also aadd the pull plots as done in #1832 and then close #1832 . Do you agree? |
@andreab1997 I added a similar function, to allow for multiple prescriptions in the same diag plot and keep the grouping by process. See for instance: if this is okay we can drop the old function, otherwise we can keep both. |
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
For me it is even fine but I am worried that in this way it would not be possible anymore to reproduce the plots that are currently in the MHOU paper |
btw @giacomomagni I guess you need to merge master to fix the tests |
If you can rebase instead of merge it would be better, that way it goes for sure on top of the last commit. |
@andreab1997 (since I guess @giacomomagni is not around this week?) do you know whether this final and should be merged? Could you have a look? |
This is final (it is just missing a review). I also wanted to add regression tests but if it is urgent, we can merge this and I will add regression tests later on. |
Nono, it is not. Please review it (and add regression tests if you can of course!) |
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
…her_thcovmat_plots
Ok did it, sorry for the delay but I was travelling. Now I can also add some more meaningfull regression tests so wait until tomorrow to start your review please (I should be able to do everything I want today). |
Thanks! Somehow in the web interface I can still see the old commits, but when I check the commits locally it seems fine so I guess it's alright. P.S. @achiefa is planning to have a first go at reviewing this |
Ok I added what I wanted to add, so please go on with the review |
Is there a report that produces plots using this PR? |
for the diagonal plots you can try:
and this runcard:
|
Thanks |
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.
Thanks, this looks good. Just some minor comments
validphys2/src/validphys/scalevariations/pointprescriptions.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
If noone complains in the next hour I will merge this |
This PR is to extend the N3LO th covmat plots @andreab1997