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

adding ParticleInCell modules #2334

Closed
wants to merge 8 commits into from
Closed

adding ParticleInCell modules #2334

wants to merge 8 commits into from

Conversation

jessdtate
Copy link
Contributor

TODO

In the new file, add your pull request template. This could include:

A reference to a related issue in your repository.
A description of the changes proposed in the pull request.
@mentions of the person or team responsible for reviewing proposed changes.

jessdtate
jessdtate commented Nov 11, 2021
View reviewed changes
src/Modules/PIC/CMakeLists.txt Outdated


SET(Modules_PIC_SRCS
CreateString.cc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete these from this directory, since we don't need to duplicate module code

jessdtate
jessdtate commented Nov 11, 2021
View reviewed changes
src/Modules/PIC/CMakeLists.txt Outdated
SET(Modules_PIC_HEADERS
CreateString.h
NetworkNotes.h
share.h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keep this file and reference

src/Modules/PIC/GravSimple.cc Outdated

#include <Modules/PIC/GravSimple.h>
#include <Core/Datatypes/String.h>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

include module header
#include<Dataflow/Network/Module.h>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the header, so don't need it here

src/Modules/PIC/GravSimple.cc Outdated
using namespace SCIRun::Core::Datatypes;
using namespace SCIRun::Dataflow::Networks;

const ModuleLookupInfo GravSimple::staticInfo_("GravSimple","PIC","SCIRun");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use macro
MODULE_INFO_DEF("GravSimple","PIC","SCIRun");

src/Modules/PIC/GravSimple.h Outdated
#define MODULES_PIC_GravSimple_H

#include <Dataflow/Network/Module.h>
#include <Modules/Fields/share.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#include <Modules/PIC/share.h>

src/Modules/PIC/share.h Outdated
#undef SCISHARE

#if defined(_WIN32) && !defined(BUILD_SCIRUN_STATIC)
#ifdef BUILD_Modules_String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#ifdef BUILD_Modules_PIC

src/Modules/PIC/GravSimple.cc Outdated
#include <Core/Datatypes/String.h>

using namespace SCIRun;
using namespace SCIRun::Modules::StringManip;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using namespace SCIRun::Modules::PIC;

src/Modules/PIC/GravSimple.h Outdated

namespace SCIRun {
namespace Modules {
namespace StringManip {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

namespace PIC {

jessdtate
jessdtate commented Nov 11, 2021
View reviewed changes
src/Modules/Factory/Config/GravSimple.module Outdated
"module":
{
"name": "GravSimple",
"namespace": "StringManip",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"namespace": "PIC",

@dcwhite
Copy link
Member

dcwhite commented Nov 11, 2021

The new code probably isn't building at all; the higher-level CMakeLists needs to know about the new directory (check src/Modules/CMakeLists.txt)

@dcwhite
Copy link
Member

dcwhite commented Nov 11, 2021

Also, please remove the duplicated modules

@dcwhite
Copy link
Member

dcwhite commented Nov 11, 2021

And also before going any further: no acronyms in the source. Every appearance of PIC should be ParticleInCell or whatever.

@dcwhite dcwhite changed the title adding PIC modules adding ParticleInCell modules Nov 11, 2021
@dcwhite dcwhite added this to the 5.0-beta.𝚵 [Internal] milestone Nov 11, 2021
@dcwhite
Copy link
Member

dcwhite commented Nov 15, 2021

Thanks, I'll check it out today

@dcwhite
Copy link
Member

dcwhite commented Nov 16, 2021

Oh so close to building--here's the fix
ad8f9a4

@jessdtate jessdtate closed this Nov 18, 2021
@jessdtate jessdtate reopened this Nov 18, 2021
@jessdtate jessdtate mentioned this pull request Nov 18, 2021
Merged
dcwhite
dcwhite reviewed Nov 19, 2021
View reviewed changes
src/Core/Algorithms/ParticleInCell/GravSimpleComputeAlgo.h Outdated
namespace SCIRun {
namespace Core {
namespace Algorithms {
namespace Math {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
namespace Math {
namespace ParticleInCell {

dcwhite
dcwhite reviewed Nov 19, 2021
View reviewed changes
src/Core/Algorithms/ParticleInCell/GravSimpleComputeAlgo.h Outdated

#include <Core/Algorithms/Base/AlgorithmVariableNames.h>
#include <Core/Algorithms/Base/AlgorithmBase.h>
#include <Core/Algorithms/Math/share.h>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include <Core/Algorithms/Math/share.h>
#include <Core/Algorithms/ParticleInCell/share.h>

dcwhite
dcwhite reviewed Nov 19, 2021
View reviewed changes
src/Core/Algorithms/ParticleInCell/GravSimpleComputeAlgo.cc Outdated
using namespace SCIRun;
using namespace SCIRun::Core::Datatypes;
using namespace SCIRun::Core::Algorithms;
using namespace SCIRun::Core::Algorithms::Math;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using namespace SCIRun::Core::Algorithms::Math;
using namespace SCIRun::Core::Algorithms::ParticleInCell;

Copy link
Contributor

Choose a reason for hiding this comment

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

You did it again, this works like a champ. Thanks, Dan!

dcwhite
dcwhite requested changes Nov 19, 2021
View reviewed changes
Copy link
Member

@dcwhite dcwhite left a comment

Choose a reason for hiding this comment

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

Just needed to be in the right namespace

@jessdtate jessdtate closed this Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kimjohn1 kimjohn1 kimjohn1 left review comments

@dcwhite dcwhite dcwhite requested changes

Assignees

@dcwhite dcwhite

@jessdtate jessdtate

Labels
External Libraries Modules
Projects
None yet
Milestone
5.0-beta.ZZZ [Conference]
Development

Successfully merging this pull request may close these issues.
3 participants
@jessdtate @dcwhite @kimjohn1