nils-braun / b2luigi

Task scheduling and batch running for basf2 jobs made simple
GNU General Public License v3.0
17 stars 11 forks source link

Automatic FEI training workflow with b2luigi using gbasf2 #94

Closed ArturAkh closed 3 years ago

codecov-commenter commented 3 years ago

Codecov Report

Merging #94 (fd171f5) into main (df77eab) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #94   +/-   ##
=======================================
  Coverage   56.19%   56.19%           
=======================================
  Files          23       23           
  Lines        1436     1436           
=======================================
  Hits          807      807           
  Misses        629      629           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update df77eab...fd171f5. Read the comment docs.

ArturAkh commented 3 years ago

@meliache

Thanks a lot for your review! I'll just very used to do os.system when I would like to do something bash-like :D

I'll try to introduce your suggestions to the example in the following days.

meliache commented 3 years ago

Thanks for the PR. I made some early comments about the code-style, mostly they concern using different os.system calls instead of using the python functions for that. In principle we are less strict with the examples as they are not part of b2luigi, this is why the tests don't cover them. Anyway, since we use the examples as advertisements, I would suggest trying for clean, idiomatic python code (also maybe PEP8-ify them with a formatter like black, yapf or autopep8).

Once the example looks nice, we can consider whether we want to include it somewhere in the online sphinx documentation under Belle II Specific Examples

meliache commented 3 years ago

As I see it, this PR is not urgent, so I might help with polishing when I have more time.

ArturAkh commented 3 years ago

Concerning this PR not being urgent: I'd like to finish the implementations (and all adjustments for better python style) by the end of this week, since from then on, I'll work on the documentation.

It is very important to make a working version of this example available to the community to run FEI on grid, therefore the time-constraints.

That being said, I'll work on further improvements until the end of the week, such that we are all satisfied with its status, but then I would like to merge this example into the main repo.

Otherwise I would have to refer to my own fork in the documenation of this example.

ArturAkh commented 3 years ago

Concerning the place of the example documentation. I think it is better to put it into the basf2 documentation, as also noted in the example README.md. Reason: this documentation on FEI is most presumably more often used, than b2luigi example, I suppose.

Of course, a link to the corresponding basf2 documentation can be put to the Belle II Specific Examples

meliache commented 3 years ago

think it is better to put it into the basf2 documentation, as also noted in the example README.md.

Then I don't understand why not include the whole example in basf2, so that you can include it in the sphinx without having to use a link? The examples here are supposed to be examples how to use b2luigi, not examples how to use the FEI.

ArturAkh commented 3 years ago

It is included here, because it is b2luigi based...

In one or the other way, I would like to avoid investing double work into documentation. And I think it is more suitable in the FEI documenation of basf2. To be discussed, as soon as I start with the documentation. We are not at that point yet.

meliache commented 3 years ago

It is included here, because it is b2luigi based...

But it is also FEI-based and basf2-based. b2luigi can be installed easily from anywhere via pip. Several people are using b2luigi in gbasf2 for their purposes, e.g. for their analyses, for the systematic corrections framework, or for the quality indicator training script that I once created. These use-cases are very specific and therefore didn't land here. I would like more examples, but for showing people how to apply b2luigi to different classes of common problems and for the purpose of the online-documentation.

Anyway, I assume that the largest work consists in creating the python files and I hope it will mean no additional work for you if you decide to publish the examples somewhere else. The sphinx documentation would also be quite portable because sphinx is sphinx.

I'm sorry for nitpicking on refactoring-issues before such a large-picture issue. I will keep the PR open for the moment while you are still working on it but please know that I'll discuss this and think whether we really need this here and I might close it in the future.

Anyway, I'm very happy and always looking forward to your contributions to b2luigi and I'd be interested to see you experiences.

Maybe in the future we could document a list of example projects that use b2luigi and then link to the FEI documentation, the systematic corrections framework etc.

ArturAkh commented 3 years ago

Yeah, I understand your point. Examples in b2luigi was for me a natural entry-point for the example to be published as code. Otherwise - to be able to document it somewhere - I would need another repository. That's also fine with me, but I thought it would be of benifit to have it within b2luigi.

Anyway, I guess I have incorporated your comments so far - thanks for being picky on the styl, only in that way I will learn proper python-style coding. In the end - if everything works as before - it looks much better than my shell hacks ^^

I'll test it once more on grid to check, whether everything works as expected and then we can see what we do with that pull request.

meliache commented 3 years ago

As discussed also outside this PR discussion we decided the example should live outside b2luigi if it's not meant for being included in the b2luigi documentation

ArturAkh commented 3 years ago

Fine with me & thanks for review :)