search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Rescale grid with bin-dependent factors #148

Closed scarlehoff closed 2 years ago

scarlehoff commented 2 years ago

In order to burn in the acceptance factor for DYE906 the option for by-bin rescaling should be available (at the moment as far as I can see I can only rescale the entire grid by a given factor, but not each bin by a different number).

cschwan commented 2 years ago

Yes, that should be possible. I'll take care of it.

cschwan commented 2 years ago

Implemented in commit 2986e576f7927875811fef77ac9cf5684377e408. @AleCandido could you please take care of the Python bindings?

alecandido commented 2 years ago

It should be implemented in 5db4c92a7b3a7f61165c6b7a107951fe23fc388b (in the sense that it is compiling, but I didn't test it).

cschwan commented 2 years ago

Is there anything missing or can I close this Issue?

scarlehoff commented 2 years ago

This won't be available until 0.53 is released, right?

cschwan commented 2 years ago

If it's too complicated to use it from git I can make a release now!

scarlehoff commented 2 years ago

For https://github.com/NNPDF/runcards/pull/139 it would be better to use an "official" release. But there's no hurry, I still need to fix the other small issues (stat uncertainties, --version, etc).

cschwan commented 2 years ago

I can make a prerelease and then @AleCandido should upgrade the version requirement for branch you mentioned. Does that work?

scarlehoff commented 2 years ago

No worries. I can wait for the release.

alecandido commented 2 years ago

I can make a prerelease and then @AleCandido should upgrade the version requirement for branch you mentioned. Does that work?

It's simple enough by me, it takes just time, and not my time: I just have to fix pyproject.toml to force the update (I could even skip in principle, but if it is only going to work with the new one it is more correct to update) and run poetry update (that might take even 30-60 mins, but I don't have to watch it continuously xD).

cschwan commented 2 years ago

@AleCandido it seems we don't support Python 3.6 anymore: https://github.com/N3PDF/pineappl/actions/runs/2515116715. Do we simply disable it?

cschwan commented 2 years ago

... and this is caused by the upgraded PyO3 crate that requires Python 3.7 at least.

felixhekhorn commented 2 years ago

I think we can just skip it, since e.g. even debain oldstable is on 3.7

alecandido commented 2 years ago

Current version of Python is 3.10, and we're getting closer to the release of 3.11. Nice to have support for as many versions as possible, but rather hard and time-consuming.

I'm happy to follow what PyO3 crate is deciding :)

cschwan commented 2 years ago

@AleCandido could you please take care of that? I think you'll have to touch the container for that, I don't see a switch in the workflow file to change the Python version on Linux.

alecandido commented 2 years ago

@AleCandido could you please take care of that? I think you'll have to touch the container for that, I don't see a switch in the workflow file to change the Python version on Linux.

We need to regenerate the container, since maturin is pre-installed there: it takes almost nothing, I only need to change https://github.com/N3PDF/pineappl/blob/e8aac12734bdd759653230339cff70c00e61840d/pineappl_py/package/Containerfile#L4 and rerun.

I was wondering if installing maturin at runtime, but I believe it is better like this: no surprise if Rust version becomes outdated or stuffs like this, we just need to generate once and essentially we can run for one year (or more, if maturin becomes more stable...).

However, instructions to regenerate the container are in the local README.md.

alecandido commented 2 years ago

Actually, removing support for 3.6 is in current beta: https://github.com/PyO3/maturin/releases/tag/v0.13.0-beta.1

Should I keep going with that one? Or what else?

EDIT: I can pass manually --interpreter option, to specify the version myself

alecandido commented 2 years ago

With just v0.13.0-beta.1 it didn't work, I'm now trying manually inside the container, to make sure it is a problem of the container and not of the workflow (not updating the container).

If it's the first case, I will just regenerate with explicit --interpreter.

cschwan commented 2 years ago

I'd say go with whatever is easier, sacrificing Python 3.6 is OK with me.

alecandido commented 2 years ago

Now I regenerated, and it is working locally.

Though it didn't in the workflow, but I don't really know which version of the container is using, since I told him to use latest (I'm considering pinning a fixed version in the workflow...)

alecandido commented 2 years ago

Ok, I rerun a bunch of times and it is not going to work.

I don't know which version of the container is running, but I know current latest to be able to compile (it is skipping Python 3.6), so I have no way to debug the action for v0.3.4 any longer.

Now I fixed the container version to the one I know is building (and so possibly working).

Please @cschwan, rerelease...

cschwan commented 2 years ago

No, still doesn't work...

alecandido commented 2 years ago

Sorry for being stupid...

I replaced the container, but the command in the workflow is overriding the one in the container...

Now I tried to release myself, but since I'm too lazy I added a script to update all the versions in 466b80179bf0289605871672579f641c47926b3. I don't know if you want to keep it, improve it, move somewhere else than top level (e.g. inside .github, or something like). Since I've done, I put it in the repo in the most obvious place (at least from my point of view).

alecandido commented 2 years ago

It finally worked, you can install also on Linux with:

pip install pineappl==0.5.3b2

Is this enough @scarlehoff? Can we close?

scarlehoff commented 2 years ago

It seems to work.