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

Seperate adapt_sources for density and pressure terms #8

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

hahahasan
Copy link
Contributor

Want to alter behaviour of the PI controller to ensure a constant power entering the SOL when performing sim scans over density

hermes-2.cxx Outdated
@@ -878,6 +899,10 @@ int Hermes::rhs(BoutReal t) {
sheath_model = 0;
}

if (ramp_j_diamag < 1.0) {
ramp_j_diamag = ramp_j_diamag_generator->generate(0, 0, 0, 20 * t/NOUT);
Copy link
Owner

Choose a reason for hiding this comment

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

Here t will be the simulation time, not the output step; NOUT is the number of steps, but t will go to NOUT * TIMESTEP. I'd prefer to just remove NOUT, since any normalisation like this can be done in the input file expression. I suggest here just have generate(0, 0, 0, t); and then change the expression in BOUT.inp.

hermes-2.cxx Outdated

// Options::root()["mesh"]["paralleltransform"].as<std::string>()
// TIMESTEP = Options::root()["TIMESTEP"].as<BoutReal>() * 1e-6;
NOUT = Options::root()["NOUT"].as<BoutReal>();
Copy link
Owner

@bendudson bendudson Apr 9, 2020

Choose a reason for hiding this comment

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

I think this (NOUT) can be removed; NOUT (and TIMESTEP) can be used in BOUT.inp expressions if needed.

hermes-2.hxx Outdated


BoutReal TIMESTEP;
BoutReal NOUT;
Copy link
Owner

Choose a reason for hiding this comment

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

Here TIMESTEP and NOUT can be removed

makefile Outdated
@@ -1,6 +1,8 @@

BOUT_TOP = ../..

BOUT_GIT_COMMIT:=$(shell git --git-dir=$(BOUT_TOP)/.git rev-parse --short HEAD >> BOUT_commit)
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this isn't used anywhere; can you see how STORM gets the commit into the executable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In STORM we have

CXXFLAGS += -DSTORM_VERSION=\"$(GIT_VERSION)\"

in the makefile.
Although this looks like it's getting the commit for BOUT rather than HERMES, but BOUT saves it's commit to the log and dump files already.

I'd suggest

Suggested change
BOUT_GIT_COMMIT:=$(shell git --git-dir=$(BOUT_TOP)/.git rev-parse --short HEAD >> BOUT_commit)
GIT_VERSION := $(shell git describe --abbrev=40 --dirty --always --tags)
CXXFLAGS += -DHERMES_VERSION=\"$(GIT_VERSION)\"

or similar (depending how you like your git hash to be formatted).

Copy link
Owner

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

It would be good to remove the atomicpp.a binary file from the history. May need the history to be squashed

hermes-2.cxx Outdated
if (slab_radial_buffers || radial_buffers) {
// Need to set the sources in the radial buffer regions to zero

if ((mesh->getGlobalXIndex(mesh->xstart) - mesh->xstart) < radial_inner_width) {
Copy link
Owner

Choose a reason for hiding this comment

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

This only does the inner buffer region; do we need something for the outer region too?

@bendudson bendudson changed the base branch from master to fci_implementation November 11, 2020 13:50
@bendudson bendudson changed the base branch from fci_implementation to master November 11, 2020 13:50
@bendudson bendudson changed the base branch from master to next November 11, 2020 13:53
@bendudson bendudson changed the base branch from next to master November 11, 2020 13:53
@johnomotani
Copy link
Collaborator

Hi Ben, looks like this branch is pulling from hahahasan:master - it doesn't look like you pushed any commits there - did you mean to change the 'from' branch?

@bendudson
Copy link
Owner

Hi @johnomotani sorry no I was a bit naughty and pushed changes to master... see the last few commits

@bendudson
Copy link
Owner

I've added a rule to protect master branch, and require PRs for future changes. Hopefully this will stop future me from pushing straight to master...

…me of the numerical diffusion terms to be variable instead of just 1; also have re-introduced slab_radial_buffers because the 3D sims seem to require this ... need to figure out why more thorughly
…tes and included the option to reverse the Curl(b/B) vector. Also had some small code tidy ups
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