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 tempdir during execution #172

Merged
merged 198 commits into from
Jan 9, 2023
Merged

Use tempdir during execution #172

merged 198 commits into from
Jan 9, 2023

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Dec 5, 2022

New implementation of #152, adapting some content from there.

@alecandido alecandido changed the base branch from master to cli December 5, 2022 18:54
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Merging #172 (d15e1d1) into cli (c32d054) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              cli     #172   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         100      100           
  Lines        4586     4587    +1     
=======================================
+ Hits         4560     4561    +1     
  Misses         26       26           
Flag Coverage Δ
isobench 53.65% <100.00%> (+0.01%) ⬆️
unittests 99.43% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/eko/__init__.py 100.00% <100.00%> (ø)

This was referenced Dec 6, 2022
@alecandido
Copy link
Member Author

Ok, I completed the massive rework of the structure, now I have to fix all tests and polish everything.

I will add the to-do list in the original message.

@alecandido alecandido mentioned this pull request Dec 8, 2022
4 tasks
@alecandido
Copy link
Member Author

alecandido commented Dec 8, 2022

Ok, now this implements also the changes that were scheduled for #160.

I'm at the level of fixing tests. I'm not sure if I should target coverage in this PR though, I fear it'll become too big...

@alecandido alecandido mentioned this pull request Dec 9, 2022
@felixhekhorn felixhekhorn added enhancement New feature or request output Output format and management labels Dec 9, 2022
@alecandido
Copy link
Member Author

alecandido commented Dec 10, 2022

I still have 38 tests failing, mostly for AttributeErrors, that are generated by the reshuffling.

At this point, features should be all available, and this PR should also close #114 and addresses part of #157

This was linked to issues Dec 10, 2022
@alecandido alecandido changed the base branch from cli to develop December 10, 2022 13:43
@alecandido
Copy link
Member Author

At this point, I decided that it is easiest to merge just this one, closing #171 #162 #161

This branch contains all of them, and it is already big enough that it doesn't change so much, including the other ones.
Moreover, a lot of changes there are partially overwritten here.

@alecandido
Copy link
Member Author

alecandido commented Dec 11, 2022

15 + 8 tests still failing, I'm getting there...

@alecandido
Copy link
Member Author

alecandido commented Dec 12, 2022

Since now we have open (with dir available) and closed (just a path) operators, a question arises.

When we compute an EKO, should we return an open one (and leave the responsibility to close it to a user that never opened it), or a closed one (and the user has to load it to use)?

@alecandido alecandido mentioned this pull request Dec 12, 2022
@alecandido
Copy link
Member Author

Last 3 unit tests missing, but for them I will take slightly more, since I want to briefly rework the legacy output.

In particular, I will drop load_yaml and dump_yaml (never used yaml format in production, so much better to get rid of one extra unused format) and dump_tar, since we will never produce new EKO in the old format.
Instead, I will make sure load_tar is actually working with the new struct, since this is the basic ingredient for the converter to the new format.

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

first batch

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

Second batch before attempting the big ones

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

I had a look to everything and with one read it seems fine ... depending on the urgency we can postpone

  • documentation is still scarse
  • tutorials are of course completely broken

to be definitely addressed before merging:

  • recover LHA benchmark

@alecandido
Copy link
Member Author

Looks like now I'm in trouble with python versions: tests are passing on 3.10, but I used some syntax that was not available in 3.8.
Most of them I can easily fix, let's hope all...

@alecandido
Copy link
Member Author

alecandido commented Dec 16, 2022

Tests are failing on py3.8 for a not very clear reason, I need further investigation, but passing on py3.9 and py3.10.
The motivations are actually only two:

ValueError: negative dimensions are not allowed
TypeError: 'numpy.float64' object cannot be interpreted as an integer

All coming from the same place: deserialization. But it is mystery why it fails only on that Python version, it would be useful to have py3.8 instance to debug, maybe I will do inside a container.

I will try to debug first isolated benchmarks, they seem to be easier to solve, and failing also on recent versions :)

@alecandido
Copy link
Member Author

Just missing coverage, that I would postpone to a later PR.

I'm ready to merge, and we are just 3 commits away from the 200 target :)

@felixhekhorn
Copy link
Contributor

Just missing coverage, that I would postpone to a later PR.

Fine - then let's do it (of course you have the pleasure of the big green button 🙃 ). However, afterwards, we have to actually put some effort to cover the remaining 140 lines ...

I'm ready to merge, and we are just 3 commits away from the 200 target :)

and with d05ca84 there are only two left ... (the improved test fails indeed without the accompanying fix)

@felixhekhorn
Copy link
Contributor

The moment we merge

@andreab1997
Copy link
Contributor

The moment we merge

Yes, I will do that. BTW remember to revert all the alpha_raw stuff here (as we agreed, but we could also open another PR of course).

@alecandido
Copy link
Member Author

alecandido commented Jan 6, 2023

I would just wait for @giacomomagni confirmation, and then merge.

Yes, I will do that. BTW remember to revert all the alpha_raw stuff here (as we agreed, but we could also open another PR of course).

@andreab1997 open a dedicated issue for the time being, and right after merging we will start the (hopefully small) PR

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

Okay green light from my side.
We might need to updated something minor also in Yadism?

@alecandido alecandido merged commit e266f66 into develop Jan 9, 2023
@alecandido
Copy link
Member Author

alecandido commented Jan 9, 2023

We might need to updated something minor also in Yadism?

Eventually, yes (at least for computing predictions without PineAPPL).
For the time being, yadism is just running with an older version of EKO (essentially for interpolation, that is almost unchanged), so you might need to environments for grids and FkTables computation, but it would work.

@alecandido alecandido deleted the tempdir branch January 9, 2023 10:20
@giacomomagni
Copy link
Collaborator

Eventually, yes (at least for computing predictions without PineAPPL). For the time being, yadism is just running with an older version of EKO (essentially for interpolation, that is almost unchanged), so you might need to environments for grids and FkTables computation, but it would work.

If I recall correctly we have already introduced eko=0.11 in yadism see here. I can take care of it.

@alecandido
Copy link
Member Author

If I recall correctly we have already introduced eko=0.11 in yadism see here. I can take care of it.

You're right, and thanks for the offering. I would suggest to pretend it is working, and as soon as anyone find an issue, we will get back to you (or any of us) and ask to make it compatible.

@felixhekhorn
Copy link
Contributor

just to say that for sure we need to do some more adjustments for eko v0.12 e.g. here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request output Output format and management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Work in a Temporary Folder Exterminate from_dict
6 participants