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

correct sorting of level1 data #1757

Closed
wants to merge 83 commits into from
Closed

Conversation

comane
Copy link
Member

@comane comane commented Jun 12, 2023

PR due to issue #1755

@comane comane added the bug Something isn't working label Jun 12, 2023
@comane comane requested a review from Zaharid June 12, 2023 16:57
@@ -369,15 +369,21 @@ def make_level1_data(data, level0_commondata_wc, filterseed, experiments_index,

indexed_level1_data = indexed_make_replica(experiments_index, level1_data)

dataset_order = [cd.setname for cd in level0_commondata_wc]
Copy link
Contributor

Choose a reason for hiding this comment

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

not that it matters much, but we should be doing this with a dict rather than a list, to avoid quadratic behavior.

# ===== create commondata instances with central values given by pseudo_data =====#
level1_commondata_dict = {c.setname: c for c in level0_commondata_wc}
level1_commondata_instances_wc = []

# note that groupby sorts alphabetically
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add sort=False to groupby. AFAICT the behaviour is not documented, as keeping the order, so I would not touch the rest in any way.

Copy link
Member Author

Choose a reason for hiding this comment

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

sort=False does not seem to solve the problem

niclaurenti and others added 19 commits June 19, 2023 14:39
Enable nf=3 with `evolven3fit_new`.
Allow categorical variables in smpdf plots
* Add luminosity channels

This adds lumi channels corresponding to the main production channels
for W and Z bosons.



---------

Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Avoid using resources at the module level
…etupfit data is written to folder using _filter_closure_data function. This function is looped over by filter_closure_data_by_experiment
@comane
Copy link
Member Author

comane commented Jun 26, 2023

Apart from the ordering of the level1 commondata list, the following bug is fixed in the present PR.
The bug is due to experiments_index and can be visualised from the snippet below

from validphys.api import API
from validphys.pseudodata import make_level1_data


inp = {
    "fit": "210623_mnc_disonly_linear_1000",
    "dataset_inputs": {"from_": "fit"},
    "theoryid":200,
    "use_cuts": "internal",
    "fakepdf": "NNPDF40_nnlo_as_01180"
}

data = API.data(**inp)
experiments_index =  API.experiments_index(**inp) # API.data_index(**inp)
level0_commondata_wc = API.level0_commondata_wc(**inp)



replica = make_level1_data(
    data=data, 
    level0_commondata_wc=level0_commondata_wc,
    filterseed=2,
    experiments_index=experiments_index,
    sep_mult=True
)




print("Ratio level 1/ level 0: all exps")
print(replica[4].central_values / level0_commondata_wc[4].central_values)

Gives:

Ratio level 1/ level 0: all exps
entry
1        4.586865
2        4.472092
3        4.856960
4        4.761477
5        4.783476
          ...    
250    205.996344
251    199.764029
252    201.027175
253    221.003710
254    207.598561
Name: data, Length: 248, dtype: float64

@comane comane requested a review from Zaharid June 26, 2023 20:40
@Zaharid
Copy link
Contributor

Zaharid commented Jun 27, 2023

@comane the diff is unreadable now unfortunately. Could you please make it so only your changes show up?

@comane
Copy link
Member Author

comane commented Jun 27, 2023

@comane the diff is unreadable now unfortunately. Could you please make it so only your changes show up?

Hi @Zaharid, not sure on how to do that. The diffs appears this way because I had to rebase the branch.
The changed files, however, are: filters.py, pseudodata.py and results.py

@RoyStegeman
Copy link
Member

When you opened this PR the additional commits were not yet in master. Perhaps changing the base branch to something else and reverting it to master corrects the diffs in this interface? Just a guess though.

The diffs appears this way because I had to rebase the branch.

Maybe I'm just misinterpreting what you mean, but there is a genuine difference between git rebase and git merge. It looks like you only merged this branch.

@Zaharid
Copy link
Contributor

Zaharid commented Jun 27, 2023

Indeed the generic approach is something like

git switch <this branch>
git rebase origin/master
git push --force-with-lease

Depending on how creative you have been in the current branch, it may be simpler to start over and git cherry-pick the relevant commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants