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

Provide guidelines to integrate external codes #27

Merged
merged 7 commits into from
Aug 6, 2020

Conversation

ziotom78
Copy link
Member

This PR integrates a section in the documentation, which should address issue #26 .

@dpole
Copy link
Member

dpole commented May 11, 2020

I skimmed through your proposal, very useful. A couple of thoughts for discussion.

It seems to me that the instructions fit perfectly the case of well-developed stand-alone public libraries that have many applications beyond LiteBIRD. I think this circumstance won't be the most frequent one. In particular, I think the instructions lean towards keeping the code outside of the repository, which shouldn't be always the case, to my view.

Here are three ideas that I feel missing, let me know what you think.

  • If a code is depeloped primarily for LiteBIRD simulations, it should be developed directly in litebird_sim as a module. More in general, if the code is developed primarily for LiteBIRD it should live in the LiteBIRD organization
  • If the code contains private information it is probably (with few exceptions) a problem because the code should get those parameters from litebrid_sims.
  • If you have a prototype routine or module that you wish to integrate, push it on a branch and open a PR to discuss with the simulation team.

@NicolettaK
Copy link
Contributor

Thanks @ziotom78!!

I have the same comment as @dpole. I think we should primarily provide guidelines on how to import in this repo codes that are specifically developed for litebird and that therefore should be copied here.

Are we going to proceed with pull requests as @dpole proposed? I think we should, but in the guidelines we should probably write down which must be the basic features that a code must have before submitting the PR.

A couple of examples:

  • how do we organize this repo? for example if I want to import here my code for map based simulations, where do I put it? Should it be a sub-package as a folder in litebird_sim/litebird_sim/?

  • Do we have any specific convention for the names of modules, functions etc. that must me adjust before submitting the pull request?

If you think it is useful I can open a PR with my module on map based sims, and use it as a test case.

@dpole
Copy link
Member

dpole commented May 18, 2020

I think that we should keep a low threshold for PR. I only see pros in sharing codes as early as possible and get feedback even in the development stage. We can place a safety net by requiring an admin to authorize any merge with master. Happy to hear about other's view, of course!

@NicolettaK
Copy link
Contributor

I believe general guidelines should be given, so that anyone who is in the phase of implementing a new piece of pipeline which will be then import here can start doing it in the proper way. In this way we can minimize the work needed from our side to approve the PR

@ziotom78
Copy link
Member Author

It would be very nice if @NicolettaK create a PR, and we all add commits to this PR to fix the document. (Yes, I agree that we should cover the case where people are going to write new code from scratch as well.)

This week I'll be busy with exams (again!), so I fear I'll not be able to put much effort in this.

@ziotom78
Copy link
Member Author

I re-read the text and found that fixing it was much less difficult than expected: apart from a few sentences here and there, the content was already appropriate for LiteBIRD-specific codes as well. The text should be ready for being integrated IMHO, but please have a check.

@ziotom78 ziotom78 merged commit 8027009 into master Aug 6, 2020
@ziotom78 ziotom78 deleted the importing_guidelines branch January 9, 2021 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants