search

madminer-tool / madminer-workflow

Madminer complete cloud-based analysis
MIT License
4 stars 4 forks source link

submodules #26 #28

Closed irinaespejo closed 4 years ago

irinaespejo commented 4 years ago

This PR adresses #26, if the PR merges into scailfin/workflow-madminer:master then the issue will be closed.

The changes introduced are:

I followed the indications in this Stack Overflow question.

Sinclert commented 4 years ago

Confusing events:

Several comments to this PR:

  1. I don't fully understand how GitHub is displaying the changes. If you review the Files changes tab, it seems that files that were already present in the repository (such as .gitignore, README, LICENSE, etc have been... re-added? 🤷‍♂️
  2. I see on the PR a long list of commits, as if the PR covers all changes happening since January 2019 until today...

The explanation for both of these events could be the introduction of GIT submodules (which I have not experience with), unless you have been playing with git rebase -i HEAD very far into the past.

Suggestions

Ignoring the confusing log of changes that GitHub is showing, I have a couple of suggestions:

  1. There is not need to include madminer-jupyter-env anymore. The purpose of that repository has no connection with the other two (which are used to define sub-workflows that will be combined to construct a bigger one).
  2. The folder where the submodules are defined (docker-images), now doesn't make sense. The defined sub-modules now point to sub-workflow definitions. So this folder can be: A) Deleted, moving madminer-workflow-ph and madminer-workflow-ml to root folder level. B) Renamed to sub_workflows or modules or ...
irinaespejo commented 4 years ago

Hi @Sinclert, I agree with your suggestions, the 206 commits is weird I'm closing this PR and starting a fresh one. Thank you!

Sinclert commented 4 years ago

I proposed an alternative PR: https://github.com/scailfin/madminer-workflow/pull/29

irinaespejo commented 4 years ago

Thank you!