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

remove unused/wrong/broken plugins #773

Merged
merged 4 commits into from
Apr 2, 2015
Merged

remove unused/wrong/broken plugins #773

merged 4 commits into from
Apr 2, 2015

Conversation

psychocoderHPC
Copy link
Member

I am currently working on plugin support for multi-species, therefor I must touch most of the plugin. To minimize the upcoming pull request I will remove some plugins.

@ComputationalRadiationPhysics/picongpu-developers
Please look to the list of removed plugins, if you think I removed to much please write here and I can delete the commit for the plugin. If you are fine with the list than also write it down.

Removed plugins:

@psychocoderHPC psychocoderHPC added the component: plugin in PIConGPU plugin label Mar 24, 2015
@PrometheusPi
Copy link
Member

  • I agree, we should remove FieldEnergy since the bugs in EnergyField are fixed with convert field energy units to Joule #502.
  • The output of IntensityPlugin would be usefull to analyze plama instabilities, since inplementing this by reading hdf5 dumps is extremly slow. But it does not work in its current state. I am pro fixing this plugin.
  • I agree that ParticleSpectrum should be removed since it is a duplication of BinEnergyParticles but does not work.
  • I agree that LiveViewPluginshould be removed since we have insituvolvis.
  • SumCurrents is probably not usefull in its current state - therefore remove.
  • I am not familiar with BinaryDensity - no opinion.
  • ParticleDensity is covered by hdf5 - remove.

@ax3l ax3l added this to the Open Beta milestone Mar 24, 2015
@ax3l ax3l self-assigned this Mar 24, 2015
@ax3l
Copy link
Member

ax3l commented Mar 24, 2015

I agree with @PrometheusPi

  • IntensityPlugin: fix (and rename it to a more useful name?)

I would also vote to keep

  • LiveViewPlugin - it's not the same as the 3D in-situ vis and connecting a client should be possible and simply needs to be done -> pls open an issue for that.
  • SumCurrents: fix, just add an MPI_Reduce there. It's very useful for single particle tests - keep it.

Ok to remove the rest, pls also remove the scripts for ParticleDensity/BinaryDensity in src/tools.

@PrometheusPi
Copy link
Member

I agree with @ax3l: renaming of IntensityPlugin is necessary as well.

@psychocoderHPC
Copy link
Member Author

IntensityPlugin:
I agree that we need a plugin which does nearly the same like IntensityPlugin but if we rename the plugin and afterwards rewrite nearly all we can remove this plugin and wrote a new one. This is easier to review than a rewrite of min. 80% of the plugin. My suggestion open an issue and remove the old plugin.

LiveViewPlugin:
We can bring back this plugin if someone had time to write a server but up to this moment this plugin is useless.

SumCurrents
We can keep this, but we need to rename it. It is not the "sum of current" it is the sum over the current change.

This was referenced Mar 25, 2015
Closed
Closed
@ax3l
Copy link
Member

ax3l commented Mar 26, 2015

👍

@psychocoderHPC
Copy link
Member Author

I removed the plugin SumCurrent from this pull request and add an issue #783 rename/refactore plugin SumCurrents

@psychocoderHPC psychocoderHPC mentioned this pull request Mar 27, 2015
Closed
@ax3l
Copy link
Member

ax3l commented Mar 27, 2015

You are still removing IntensityPlugin in this pull. Pls revert that change.
Same for the tool src/tools/bin/plotIntensity.

A comment on the LiveViewPlugin -> the moment you remove it, it will get outdated with data structures and everything that changes very soon. That means you will never "bring it back". That is the reason I do not like to remove it. So at least keep the plugin in, so only the server is outdated. Else you will have to update both at some time in the future, instead of getting the updates and CI support during the development from every developer that changes code - it is still a bad idea to remove it.

@psychocoderHPC
remove plugin FieldEnergy
db41ae5 
- delete `FieldEnergy.hpp`
- delete `FieldEnergy.tpp`
- remove `FieldEnergy` from `PluginController.hpp`
@psychocoderHPC
remoove plugin ParticleSpectrum
aee5a41 
- delete plgin
- remove plugin from `PluginController`
@psychocoderHPC
remove plugin/tools for binary density
4e149a7 
- delete plugin `ParticleDensity`
- delete tool `plotDensity`
@psychocoderHPC
Copy link
Member Author

removed plugin from this pull request:

  • LiveViewPlugin
  • IntensityPlugin

@psychocoderHPC
remove plugin ParticleDensity
73abb88 
- delete plugin
- remove plugin usage in `PluginController`
@ax3l
Copy link
Member

ax3l commented Mar 31, 2015

Current change in plugins:

  • FieldEnergy
  • ParticleDensity
  • BinaryDensity
  • ParticleSpectrum

looks ok - I'll just wait for the CI to confirm.

ax3l added a commit that referenced this pull request Apr 2, 2015
@ax3l
Merge pull request #773 from psychocoderHPC/topic-removePlugins
118d636 
remove unused/wrong/broken plugins
@ax3l ax3l merged commit 118d636 into ComputationalRadiationPhysics:dev Apr 2, 2015
ax3l
ax3l reviewed Apr 2, 2015
View reviewed changes
src/picongpu/include/plugins/PluginController.hpp

#if(ENABLE_HDF5 == 1)
/* speciesParticleShape::ParticleShape::ChargeAssignment */
typedef PhaseSpaceMulti<particles::shapes::Counter::ChargeAssignment, PIC_Electrons> PhaseSpaceElectrons;
#endif
#if(SIMDIM==DIM3)
#if(PIC_ENABLE_PNG==1)
typedef heiko::ParticleDensity<PIC_Electrons> HeikoParticleDensity;
Copy link
Member

Choose a reason for hiding this comment

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

@f-schmitt-zih we just removed the heiko:: namespace. can you believe that? ;)

@psychocoderHPC psychocoderHPC deleted the topic-removePlugins branch September 8, 2016 11:52
This was referenced Nov 11, 2016
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ax3l ax3l

Labels
component: plugin in PIConGPU plugin
Projects
None yet
Milestone
0.2.0: Open Beta
Development

Successfully merging this pull request may close these issues.
3 participants
@psychocoderHPC @PrometheusPi @ax3l