Restore needed thcovmat functions#1901
Conversation
There was a problem hiding this comment.
I guess the important question is, do we need this for the papers and do we need this quick?
I might be able to work tomorrow on changing the functions that use arrays to use dataframes instead (if you point me to them).
As I said in the comments, I'm not convinced covmap is correct unless the order in the runcard is the "right one".
edit: note that the need for all this trickery was probably due to the mix of C++ and python. Now that in vp we can have dataframes all way through you should not need any mappings because the dataframe contains the mapping.
Unfortunately I think we need this plot (ie the red and black shift plot) for the papers. 😭 Thanks you all for the help. |
|
Only |
I think so, running the previous vp-runcard I'm getting: |
|
Can you share your runcard? |
here you go: |
|
Do we know why is the theorycovmat shift calculated using the diagonal elements of the theory covmat instead of e.g. the values on the diagonal of the covmat with all variations considered? I know both neglect part of the picture and I'm not sure which is the better choice for interpretation/presentation, but do you know the motivation for this chioce? |
Personally I don't, this looks more something that happens when you start with a basic setup and then you add features but you forgot to update everything.
I'd say we can fix this behaviour and always compute the full covmat. If only the diagonal is needed you extract them from the full matrix. Maybe @andreab1997 or someone else have better ideas. |
I was indeed thinking along those lines |
No we need all the functions in |
|
Just to keep track of this: Now the function I leave here a couple of runcard that you can use (given that I simplified their structure a bit): |
|
Ok maybe the problem was just the one corrected in 52f3262 but if you can please have a look anyway |
Do you mean the problem is with the labels of the stored covmat from vp-setupfit or only in these plotting function? I'm not so sure what you'd like us to check, could you share the plot/table you're worried about? |
No, after 52f3262 maybe there is no problem. For example this is the new report for DYCC to be compared with the old one DYCC. Of course they are not the same and they should not given that the old one was affected by all the ordering problems, but now they look reasonably similar. Anyway, I will ask you for a review but if you want to have a look at the code even now, please do it even if for the moment it is just a draft. |
giacomomagni
left a comment
There was a problem hiding this comment.
LGTM and it's working!
|
@scarlehoff I resolved all your conversations because they refer to functions that I now removed (5db9ae0), together with all the others that are not used anymore in the theorycovariance module. Please check if in 5db9ae0 I removed something that you use or that I should not have removed. |
scarlehoff
left a comment
There was a problem hiding this comment.
I'm completely happy with anything that implies removing code from this module :P
|
This is the complete test report that we need for the papers as obtained with the current status of this branch: https://vp.nnpdf.science/MLy8yUWdRyOqJpEYERiEmg== . Results are very similar to old ones but of course not completely identical given that we solved the ordering issue. I am now about to check and improve everything again in CC: @giacomomagni (I don't know to be honest if you need all the tests but in case here it is) |
Regression tests. Once you have a result which you know for sure that it is correct, add a regression test. You can use the theories from here https://docs.nnpdf.science/vp/examples.html?highlight=resources#recommended-resources |
|
Given that from my side everything is ready here (excluding regression tests), I would say that this is ready to be reviewed. @scarlehoff about regression tests I was thinking about comparing reports (given that the tests pourpose is exactly producing a validation report). How do you suggest to do? |
Instead of comparing reports you could compare plots (or tables with info used to construct the plots) |
|
Yes. I'd suggest comparing dataframes (there's a few examples in the tests). You can compare for instance theory covmat with a few edge cases that you know would fail for a problematic ordering. |
RoyStegeman
left a comment
There was a problem hiding this comment.
Please make sure to not leave any dead code after removing functions. I only pointed out two of them, but I didn't check all functions so there may be more.
RoyStegeman
left a comment
There was a problem hiding this comment.
Okay I didn't look at it super close but given the results I'll trust that it works. We don't want to wait a long time with merging this either.
Ok then I would say that if @giacomomagni can try again now his usual plots just to be sure, then we can merge. Note that I am about to do just another commit for cosmetic changes so wait for that. |
|
I'm happy with this PR, but unfortunately some changes are still required (at least for the N3LO): In particular: shall I give a try to implement them here? |
I would say, let's merge this now in such a way we can tag the code and then let's open another PR for these changes you are asking for. Do you agree? |
It seems that #1899 removed some functions that are needed for some of the reports we usually do (cc @giacomomagni). For example for
shift_diag_cov_comparison. I am trying to restore and eventually fix this here. @RoyStegeman @scarlehoff