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

Changing the eko argument in evolve_grid #28

Merged
merged 10 commits into from
Jul 14, 2022
Merged

Changing the eko argument in evolve_grid #28

merged 10 commits into from
Jul 14, 2022

Conversation

andreab1997
Copy link
Contributor

We want to pass the eko object rather than the path to evolve_grid

@andreab1997
Copy link
Contributor Author

@felixhekhorn I am not sure but maybe it is better to merge this to #24 rather than to main. Am I wrong?

@felixhekhorn felixhekhorn changed the base branch from main to sv_check July 8, 2022 10:36
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #28 (5a2f9ce) into main (aa1a068) will increase coverage by 0.11%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   40.75%   40.86%   +0.11%     
==========================================
  Files          15       15              
  Lines         530      531       +1     
==========================================
+ Hits          216      217       +1     
  Misses        314      314              
Flag Coverage Δ
unittests 40.86% <50.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
src/pineko/evolve.py 30.61% <ø> (+0.61%) ⬆️
src/pineko/theory.py 19.87% <ø> (ø)
src/pineko/cli/convolute.py 75.00% <50.00%> (-2.28%) ⬇️

@felixhekhorn
Copy link
Contributor

Can this then be merged? or we wait for #24 to be merged first?

@alecandido
Copy link
Member

Before rebase, please

@andreab1997
Copy link
Contributor Author

Before rebase, please

on which branch?

@andreab1997
Copy link
Contributor Author

Before rebase, please

on which branch?

to main I guess

@felixhekhorn
Copy link
Contributor

Before rebase, please

as you know, I don't like this "rewriting history" business 🙃 so I started to reread the reason why people are doing this and the ("simple") rule seems to be "You can get the best of both worlds: rebase local changes before pushing to clean up your work, but never rebase anything that you’ve pushed somewhere." aka "only rebase branches you, and only you, are working on."

so I'd say there should not be a rebase visible on github - hence you can just merge sv_check here (and before merge master on sv_check)

@alecandido
Copy link
Member

alecandido commented Jul 11, 2022

to main I guess

On the one your conflicting with, i.e. the one on which the PR is based:

image

I'd say sv_check then.

@andreab1997
Copy link
Contributor Author

Can this then be merged? or we wait for #24 to be merged first?

I believe we can merge now, I solved the conflicts

@alecandido
Copy link
Member

so I'd say there should not be a rebase visible on github - hence you can just merge sv_check here (and before merge master on sv_check)

I'm not saying you should do what I wish, if you do not agree with me, do whatever you wish. But I'll keep doing as well :)

The points are the following:

  1. linear histories are simpler: when you want to explore when changes entered, there is a before and an after; at the time you merge your main was arrived at some point, so the commit you're inserting are arriving afterwards - this is simple, we solve only at merge time
  2. if you do locally, then you never do: we always push everything, just to keep other collaborators updated, so it's always remote

Baseline: I will keep rebasing, feel free to do whatever you think it's best :)

@andreab1997
Copy link
Contributor Author

So at the end I am a bit confused, what do we want to do with this?

@alecandido
Copy link
Member

alecandido commented Jul 12, 2022

So at the end I am a bit confused, what do we want to do with this?

I have an opinion, @felixhekhorn has the opposite. Pick your favorite for your own reasons :)

(However, either merge or rebased, but something has to be done, on this we agree)

@felixhekhorn felixhekhorn added the refactor Refactor code label Jul 12, 2022
@delete-merged-branch delete-merged-branch bot deleted the branch main July 12, 2022 15:22
@felixhekhorn felixhekhorn changed the base branch from sv_check to main July 12, 2022 16:12
@andreab1997
Copy link
Contributor Author

Is everything ok with this?

@alecandido alecandido mentioned this pull request Jul 13, 2022
@andreab1997 andreab1997 merged commit 2dc827f into main Jul 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the change_args branch July 14, 2022 12:21
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