Skip to content

adding diagonal/blockdiagonal functionalities, round 2#467

Merged
Zaharid merged 5 commits into
masterfrom
diagonal_thcovmat
Jun 4, 2019
Merged

adding diagonal/blockdiagonal functionalities, round 2#467
Zaharid merged 5 commits into
masterfrom
diagonal_thcovmat

Conversation

@tgiani
Copy link
Copy Markdown
Contributor

@tgiani tgiani commented May 21, 2019

Here as agreed I implement again the same things as in PR #438

@tgiani
Copy link
Copy Markdown
Contributor Author

tgiani commented May 21, 2019

There s something I don t understand. If I define the function theory_diagonal_covmat as

def theory_diagonal_covmat(theory_covmat_singleprocess,experiments_index):
    """Returns theory covmat with only diagonal values"""
    s = theory_covmat_singleprocess.values
    # Initialise array of zeros and set precision to same as FK tables
    s_diag = np.zeros((len(s),len(s)), dtype=np.float32)
    np.fill_diagonal(s_diag, np.diag(s))
    df = pd.DataFrame(s_diag, index=experiments_index, columns=experiments_index)
    return df

which is just as in the same way as in #438, I get the following error

[CRITICAL]: Bug in setup-fit ocurred. Please report it.
Traceback (most recent call last):
  File "/scratch/nnpdfgit/nnpdf/validphys2/src/validphys/scripts/vp_setupfit.py", line 181, in run
    super().run()
  File "/scratch/nnpdfgit/nnpdf/validphys2/src/validphys/app.py", line 143, in run
    super().run()
  File "/scratch/anaconda2/envs/nnpdf/lib/python3.7/site-packages/reportengine/app.py", line 359, in run
    rb.execute_sequential()
  File "/scratch/anaconda2/envs/nnpdf/lib/python3.7/site-packages/reportengine/resourcebuilder.py", line 168, in execute_sequential
    perform_final=self.perform_final)
  File "/scratch/anaconda2/envs/nnpdf/lib/python3.7/site-packages/reportengine/resourcebuilder.py", line 175, in get_result
    fres =  function(**kwdict)
  File "/scratch/nnpdfgit/nnpdf/validphys2/src/validphys/results.py", line 184, in experiment_result_table_no_table
    data_result = DataResult(loaded_exp)
TypeError: __init__() missing 2 required positional arguments: 'covmat' and 'sqrtcovmat'

which I was no getting before. Does anyone know why?
Btw I ve changed the definition of theory_diagonal_covmat to use theory_block_diag_covmat instead of theory_covmat_singleprocess and in this way the crash is not happening

@tgiani tgiani requested review from RosalynLP and Zaharid May 21, 2019 08:51
@tgiani
Copy link
Copy Markdown
Contributor Author

tgiani commented May 21, 2019

@RosalynLP I ve added also your commit where you changed the names of the functions. Please check if I ve done something stupid

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 21, 2019

This bug was introduced in #427 and is being addressed by #447.

cc @wilsonmr

Copy link
Copy Markdown
Contributor

@RosalynLP RosalynLP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgiani looks sensible to me :)

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 21, 2019

@tgiani You should be able to rebase this on top of master and get it to work.

@tgiani tgiani force-pushed the diagonal_thcovmat branch from 7d80e89 to f4aafea Compare May 22, 2019 15:19
Comment thread validphys2/src/validphys/config.py Outdated

@configparser.explicit_node
def produce_nnfit_theory_covmat(self, use_thcovmat_in_sampling:bool, use_thcovmat_in_fitting:bool):
def produce_nnfit_theory_covmat(self, use_thcovmat_in_sampling:bool, use_thcovmat_in_fitting:bool,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a docstring here explaining what everything does. Also mention the existence of the flag in the corresponding section of guide.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 31, 2019

@tgiani I have described the options explicitly, so that it is possible to use this by reading the documentation. I have also run a formatting tool to fix various whitespace errors.

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 31, 2019

Hello, this is @Zaharid's automated QA script. Please note it is highly experimental. I ran pylint on your changes and found some new issues.

On validphys2/src/validphys/config.py, pylint has reported the following new issues:

  • Line 706: Unused argument 'use_thcovmat_in_sampling'
  • Line 707: Unused argument 'use_thcovmat_in_fitting'

On validphys2/src/validphys/theorycovariance/construction.py, pylint has reported the following new issues:

  • Line 67: Unused argument 'theoryids'
  • Line 68: Unused argument 'fivetheories'
  • Line 78: Unused argument 'fivetheories'
  • Line 78: Line too long (109/100)
  • Line 464: Trailing whitespace
  • Line 488: Trailing whitespace
  • Line 523: Trailing whitespace

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented May 31, 2019

Please fix the formatting errors in construction.py.

@tgiani
Copy link
Copy Markdown
Contributor Author

tgiani commented Jun 1, 2019

@Zaharid Uhm do you mean

Line 464: Trailing whitespace
Line 488: Trailing whitespace
Line 523: Trailing whitespace

from your automated script?

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jun 3, 2019

Yes, and also "line too long".

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jun 4, 2019

Hello, this is @Zaharid's automated QA script. Please note it is highly experimental. I ran pylint on your changes and found some new issues.

On validphys2/src/validphys/config.py, pylint has reported the following new issues:

  • Line 706: Unused argument 'use_thcovmat_in_sampling'
  • Line 707: Unused argument 'use_thcovmat_in_fitting'

On validphys2/src/validphys/theorycovariance/construction.py, pylint has reported the following new issues:

  • Line 67: Unused argument 'theoryids'
  • Line 68: Unused argument 'fivetheories'
  • Line 79: Unused argument 'fivetheories'

@Zaharid Zaharid merged commit b1de699 into master Jun 4, 2019
@Zaharid Zaharid mentioned this pull request Jun 4, 2019
@Zaharid Zaharid deleted the diagonal_thcovmat branch June 11, 2019 10:26
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.

3 participants