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.

Sign up for GitHub

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

Jump to bottom

CONTRIBUTING: Update/Modernize #2600

Merged
merged 2 commits into from
Nov 30, 2021
Merged

CONTRIBUTING: Update/Modernize #2600

merged 2 commits into from
Nov 30, 2021

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Nov 23, 2021 •
edited
Loading

  • Add GitHub account setup
  • Add local git setup
  • Modernize for CMake workflows

Same as BLAST-ImpactX/impactx#16

@ax3l ax3l added the component: documentation Docs, readme and manual label Nov 23, 2021
@ax3l ax3l requested a review from RemiLehe November 23, 2021 23:42
@ax3l ax3l force-pushed the doc-updateContributing branch from db737ff to 27761a8 Compare November 23, 2021 23:43
@ax3l
CONTRIBUTING: Update/Modernize
62d79aa 
- Add GitHub account setup
- Add local git setup
- Modernize for CMake workflows
@ax3l ax3l force-pushed the doc-updateContributing branch from 27761a8 to 62d79aa Compare November 23, 2021 23:46
@ax3l ax3l requested a review from EZoni November 23, 2021 23:46
@ax3l ax3l mentioned this pull request Nov 23, 2021
Merged
EZoni
EZoni reviewed Nov 24, 2021
View reviewed changes
Copy link
Member

@EZoni EZoni left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, Axel! I left only a few minor comments.

CONTRIBUTING.rst

The commit message (between quotation marks) is super important in order to follow the developments and identify bugs.
The most important part is really that you find a descriptive title
and add an empty newline after it.

Copy link
Member

Choose a reason for hiding this comment

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

Here we could add something like: "If you realize that you made a mistake in your commit message and want to change it, before pushing the commit, you can do this with git commit --amend, which will open the commit message in an editor and let you modify it."

Copy link
Member Author

@ax3l ax3l Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that's too advanced for here. Multiple commits do not hurt for beginners as we squash them on merge.

History rewriting (amend, rebase) are advanced git topics and I don't want people to footgun themselves by starting right away with push -f when they are first time using git.

git rm of some files is non-destructive during review, but push -f can potentially remove some work that they did not backup.

ax3l
ax3l commented Nov 24, 2021
View reviewed changes
ax3l
ax3l commented Nov 24, 2021
View reviewed changes
ax3l
ax3l commented Nov 24, 2021
View reviewed changes
ax3l
ax3l commented Nov 24, 2021
View reviewed changes
ax3l
ax3l commented Nov 24, 2021
View reviewed changes
@ax3l @EZoni
Apply suggestions Edoardo
e5a4b6c 
Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com>
@ax3l ax3l mentioned this pull request Nov 24, 2021
Merged
EZoni
EZoni approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@EZoni EZoni left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for updating our guidelines!

@ax3l ax3l enabled auto-merge (squash) November 24, 2021 17:10
@EZoni
Copy link
Member

EZoni commented Nov 29, 2021

I see that the Python test Python_particle_attr_access_unique is failing here, which is odd. Needs to be understood if the test was broken somewhere else and we just need to rebase on development or if it's just some kind of glitch.

@ax3l
Copy link
Member Author

ax3l commented Nov 30, 2021

Likely pre-existing problem with the test, since this PR does not change the test.

Restarted CI now.

@ax3l ax3l merged commit 8e9e8e8 into ECP-WarpX:development Nov 30, 2021
@ax3l ax3l deleted the doc-updateContributing branch November 30, 2021 16:39
@ax3l
Copy link
Member Author

ax3l commented Nov 30, 2021

Might be addressed with #2580

roelof-groenewald added a commit to ModernElectron/WarpX that referenced this pull request Dec 3, 2021
@roelof-groenewald @ax3l
@EZoni @RemiLehe @lgiacome @KZhu-ME @peterscherpelz @dpgrote @PhilMiller @s9105947 @tropf @prkkumar
Update memaster (#110)
033b51e 
* Fix Init of Vector Members (ECP-WarpX#2595)

Fix default init of `Vector` member variables. The old construct
is not valid C++.

https://stackoverflow.com/a/11491003/2719194

* C++17: Work-Around NVCC gatherParticles (ECP-WarpX#2596)

The `noexcept` lambda does not compile in C++17 mode due to an NVCC compiler bug, at least in NVCC 11.3.109. Compiles in C++14 mode with the same compiler.

* requirements.txt - PICMI development version (ECP-WarpX#2588)

Document in `requirements.txt` on how to install a pre-release
version of PICMI.

* CONTRIBUTING: Update/Modernize (ECP-WarpX#2600)

* CONTRIBUTING: Update/Modernize

- Add GitHub account setup
- Add local git setup
- Modernize for CMake workflows

* Apply suggestions by Edoardo

Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com>

* Replaced duplicated current deposition documentation (ECP-WarpX#2604)

* Throwing a warning if particle_shape>1 with EB (ECP-WarpX#2592)

* Aborting if particle_shape!=1 with EB

* Throw warning instead of aborting

* Checking at runtime if EB is initialized

* Added missing preprocessor directive

* Ignoring an unused variable

* Fix typo

* Improve style

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>

* Fix segfault when importing _libwarpx without initializing WarpX (ECP-WarpX#2580)

* Added check for if warpx was initialized when calling finalize

* Renamed to be warpx_initialized

* Fixed reference to global variable

Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com>

* Changed global variable to member of libwarpx

* Fixed syntax errors

* Remove custom arg from argv to avoid parmparse error

Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com>

* Added parallel pragma to ApplyBoundaryConditions (ECP-WarpX#2612)

* Note that CCache 4.2 introduced large CUDA improvements (ECP-WarpX#2606)

* Dimensionality Docs: Default (ECP-WarpX#2609)

Just adds the note that 3D is the default geometry.

* AMReX: Weekly Update (ECP-WarpX#2613)

* MergeBuffersForPlotfile: Barrier (ECP-WarpX#2608)

Make sure that all MPI ranks are in sync, i.e., have closed the
files that they wrote, before trying to merge them.

* Fix installation location for libraries (ECP-WarpX#2583)

During configuration the installation location for libraries is given by
dumping the cmake variable `CMAKE_INSTALL_LIBDIR`.
This commit adjusts the installation of WarpX libraries (WarpX_LIB=ON)
to respect this setting.

Co-authored-by: Rolf Pfeffertal <tropf@users.noreply.github.com>

* Release 21.12 (ECP-WarpX#2614)

* AMReX: 21.12

* PICSAR: 21.12

* WarpX: 21.12

* div(E,B) Cleaning Options for PSATD (ECP-WarpX#2403)

* Implement div(E)/div(B) Cleaning with Standard PSATD

* Cleaning

* Update Benchmark

* Add Nodal Synchronization of F,G

* OneStep_multiJ: Nodal Syncs, Damp PML

* OneStep_multiJ: Push PSATD Fields in PML

* div Cleaning Defaults (Domain v. PML)

* Include Fix of ECP-WarpX#2429 until Merged

* Reset Benchmark of Langmuir_multi_psatd_div_cleaning

* Multi-J: Remove PML Support

* Include Fix of ECP-WarpX#2474 Until Merged

* Exchange All Guard Cells for F,G

* Fix Defaults

* Update Test, Reset Benchmark

* Fix Defaults

* Cleaning

* Default update_with_rho=1 if do_dive_cleaning=1

* Update CI Test pml_psatd_dive_divb_cleaning

* Replace Warning with Abort

* Add 2D Langmuir Test w/ MR & PSATD (ECP-WarpX#2605)

* Add 2D Langmuir Test w/ MR & PSATD

* Add Missing Compile String

* Fix out-of-bound in Inverse FFT of F,G (ECP-WarpX#2619)

* Mention that the potentail should be constant inside EB (ECP-WarpX#2618)

* Mention that the potentail should be constant inside EB

* Update text

* Replace AMREX_SPACEDIM: Boundary & Parallelization (ECP-WarpX#2620)

* AMREX_SPACEDIM : Boundary Conditions
* AMREX_SPACEDIM : Parallelization
* Fix compilation
* Update Source/Parallelization/WarpXComm_K.H

* Fix out-of-bound in the initialization of EB (ECP-WarpX#2607)

* Call FillBoundary when initializing EB

* Avoid out-of-bound

* Bug fix

* Apply suggestions from code review

* update version number

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com>
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
Co-authored-by: Lorenzo Giacomel <47607756+lgiacome@users.noreply.github.com>
Co-authored-by: Kevin Z. Zhu <86268612+KZhu-ME@users.noreply.github.com>
Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com>
Co-authored-by: David Grote <grote1@llnl.gov>
Co-authored-by: Phil Miller <phil@intensecomputing.com>
Co-authored-by: s9105947 <80697868+s9105947@users.noreply.github.com>
Co-authored-by: Rolf Pfeffertal <tropf@users.noreply.github.com>
Co-authored-by: Prabhat Kumar <89051199+prkkumar@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EZoni EZoni EZoni approved these changes

@RemiLehe RemiLehe Awaiting requested review from RemiLehe

Assignees

@EZoni EZoni

Labels
component: documentation Docs, readme and manual
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ax3l @EZoni