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

Fix struct.detached #161

Closed
wants to merge 3 commits into from
Closed

Fix struct.detached #161

wants to merge 3 commits into from

Conversation

andreab1997
Copy link
Contributor

We want output.struct.detached to work even if the eko does not contain inputpids, targetpids, inputgrid and targetgrid, as this will be the case from now on. @felixhekhorn @alecandido

@andreab1997 andreab1997 changed the base branch from master to develop November 8, 2022 14:27
@felixhekhorn felixhekhorn added the refactor Refactor code label Nov 8, 2022
del bases[basis]
if basis in bases:
bases[f"_{basis}"] = bases[basis]
del bases[basis]
bases["pids"] = np.array(br.flavor_basis_pids)
for k in ("xgrid", "_inputgrid", "_targetgrid"):
Copy link
Member

@alecandido alecandido Nov 8, 2022

Choose a reason for hiding this comment

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

Dropping the former loop, also this loop has to be dropped. Essentially, replace k everywhere with "xgrid"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I did before but then the tests are really a problem. Of course fixing the tests would be the correct solution but, since at the moment we are not using at all rotations, I believe that in the next future we need to get rid completely of it. This will be the proper change and at that moment we can drop these loops and fix the tests accordingly.

BTW I am saying that we are not using at all rotations because even if I use x_grid_reshape and it changes targetgrid, then load() would completely ignore targetgrid and would set it to xgrid (wrongly because instead the operator is rotated). This is the problem that I have with pineko. So now rotations is not even keeping track of the rotations. It is completely useless.

Copy link
Member

@alecandido alecandido Nov 8, 2022

Choose a reason for hiding this comment

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

BTW I am saying that we are not using at all rotations because even if I use x_grid_reshape and it changes targetgrid, then load() would completely ignore targetgrid and would set it to xgrid

This should not happen: we want to "ignore" targetgrid (i.e. not putting it at all) only in the runcard. The reason why it is failing in Pineko is different: metadata are not automatically updated, and you are not doing it specifically.
Metadata are one thing, and runcards are another. Incidentally, they both have rotations - but we can drop rotations at all from the runcard, since we'll never use it any longer.

EDIT: this is partially the reason why I'm pressing for #160, since at the moment there is a huge clash

@andreab1997
Copy link
Contributor Author

So I removed the loops as agreed yesterday and started to fix the tests. However, as you can see, this is not as trivial as removing every mention (but the ones in legacy) of targetgrid, inputgrid, inputpids and targetpids in conftest.py. I will try to understand the situation better but any suggestion will be welcome :) @alecandido @felixhekhorn

operator["rotations"][k],
log=operator["configs"]["interpolation_is_log"],
)
if operator["rotations"]["xgrid"] is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm not sure any longer with need this if: xgrid should not be allowed to be None.

Isn't it? @felixhekhorn

@andreab1997
Copy link
Contributor Author

I believe that the tests that we are not passing now can only be fixed after we allow for the metadata file we agreed yesterday. I am now opening that PR

@alecandido
Copy link
Member

This PR updates EKO.detached, but in #172 the function has been killed completely, so the PR is already outdated.

I would close without merging.

@andreab1997
Copy link
Contributor Author

This PR updates EKO.detached, but in #172 the function has been killed completely, so the PR is already outdated.

I would close without merging.

Fine for me. The only things we want to keep are the conftest fixes I believe but they should be trivial. Anyway note that #162 is based on this PR

@alecandido
Copy link
Member

Yes, this is the first of the tower, since #161 -> #162 -> #171 -> #172.

So #172 already contains everything that is here, but dropped EKO.detached.
In a sense, I'm proposing to "merge forward" along the tower.

@alecandido alecandido mentioned this pull request Dec 10, 2022
20 tasks
@felixhekhorn
Copy link
Contributor

Agreed - let's close without merging.

In a sense, I'm proposing to "merge forward" along the tower.

you mean close all PRs and change the target branch of the last?

@alecandido alecandido closed this Dec 12, 2022
@felixhekhorn felixhekhorn deleted the Fix_struct_detached branch January 5, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants