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

use error_members instead of rawdata #1517

Merged
merged 5 commits into from
Mar 1, 2022

Conversation

scarlehoff
Copy link
Member

This is straightforward for MC PDFs (they are the same thing now and this change will make the change from all_members to only_replicas very simple) but for Hessian PDFs is instead very dangerous since the error members are the rawdata-1.

Making this as a separate PR since it makes many changes in the code but are almost a sed (a manual one nonetheless) and because I will be running the fit bot quite a bit I guess.

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Feb 14, 2022
@scarlehoff
Copy link
Member Author

@Zaharid are you using the paramfits package? Is it tested / are there runcards that I can use as an example that things don't break? (the mean, std and raw .data is used a lot there)

@github-actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Feb 14, 2022
@scarlehoff
Copy link
Member Author

oh, that's a pontetially important one, didn't realise it was paramfits stuff.

Maybe using Stats everywhere will create the parabolas :P

@Zaharid
Copy link
Contributor

Zaharid commented Feb 14, 2022

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Feb 15, 2022
@scarlehoff
Copy link
Member Author

The error seems to be reportengine not knowning what to do with a numpy array here: https://github.com/NNPDF/reportengine/blob/fe736351c94b1f0a24b9c695da5bffafa1e5e98b/src/reportengine/floatformatting.py#L80 I think this case should be taken into account (maybe with a nicely-formatted error) since it seems to accept them.

That said, the fact that now they are arrays (of one element) when they used to be floats is a side-effect I was not expecting so I'm actually happy it failed.

@scarlehoff
Copy link
Member Author

Sorry, this one https://vp.nnpdf.science/dKVAidYgQRW9hPBGQ9z3Rw==/input/runcard.yaml

Ok, for now it seems to work and generate the same thing. Thanks!

@scarlehoff scarlehoff marked this pull request as ready for review February 15, 2022 15:30
@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. destroyingc++ labels Feb 15, 2022
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Feb 16, 2022
@Zaharid
Copy link
Contributor

Zaharid commented Feb 24, 2022

The error seems to be reportengine not knowning what to do with a numpy array here: https://github.com/NNPDF/reportengine/blob/fe736351c94b1f0a24b9c695da5bffafa1e5e98b/src/reportengine/floatformatting.py#L80 I think this case should be taken into account (maybe with a nicely-formatted error) since it seems to accept them.

That said, the fact that now they are arrays (of one element) when they used to be floats is a side-effect I was not expecting so I'm actually happy it failed.

How did you trigger that?

@scarlehoff
Copy link
Member Author

How did you trigger that?

from reportengine.floatformatting import ValueErrorTuple
import numpy as np 
aa = ValueErrorTuple(np.array([3]), np.array([1]))
print(aa)

I don't think throwing an exception here is a bug as it did help me, but naively I would think it is a common enough situation that warrants its own exception.

@Zaharid
Copy link
Contributor

Zaharid commented Feb 24, 2022

I'd say it is behaving as expected. Not adding that special case.

@Zaharid
Copy link
Contributor

Zaharid commented Feb 24, 2022

This looks good as well. Not testing too much manually but then again hopefully we have automated tests for most stuff.

Base automatically changed from utilize_stats_rounds_one to master March 1, 2022 11:24
@scarlehoff scarlehoff merged commit d3b2639 into master Mar 1, 2022
@scarlehoff scarlehoff deleted the utilize_error_members_instead_of_rawdata branch March 1, 2022 11:24
@Zaharid Zaharid added the enhancement New feature or request label May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
destroyingc++ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants