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

Ionosphere commits #56

Open
wants to merge 40 commits into
base: ionosphere
Choose a base branch
from
Open

Conversation

ykempf
Copy link
Collaborator

@ykempf ykempf commented Feb 22, 2022

Here's a string of commits I made of all my debugging and developments of yesterday. I suggest cherry-picking one by one.

e354eac was so much work I'd like to have it in. Makes it safer, really...
5462ca0 is cosmetic, but doesn't harm either.

a62574f is the big one.

3333680 is probably better to have in.

56e0020 and 39498b2 are features from gumics, might be useful for further testing and benchmarking.

@ykempf
Copy link
Collaborator Author

ykempf commented Feb 22, 2022

Also, I really suggest to do them one by one as e354eac is such a monster.


// If we somehow still map into the ionosphere, we missed the 88 degree criterion but shouldn't couple there.
if(sqrt(x.at(0)*x.at(0) + x.at(1)*x.at(1) + x.at(2)*x.at(2)) < Ionosphere::innerRadius) {
# TODO drop this warning if it never occurs? To be followed.
Copy link
Owner

Choose a reason for hiding this comment

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

You, sir, are writing too much python. This is not a c++ source code comment. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hupsis. I wanted to make that a #warning, or was it #pragma warning? I'll fix it.

@ursg
Copy link
Owner

ursg commented Feb 22, 2022

Ok, merged 56e0020, 39498b2 and a62574f already. The other ones I will look through tomorrow. Great work!

@ykempf
Copy link
Collaborator Author

ykempf commented Feb 23, 2022

The last two are waiting to run on Mahti.

…e-commits

Conflicts:
	sysboundary/ionosphere.cpp
@ursg
Copy link
Owner

ursg commented Feb 23, 2022

Also merged 3333680 (and 93223d6 to fix it) now, as well as f3a77b0 and f2b9abd.

// If we somehow still map into the ionosphere, we missed the 88 degree criterion but shouldn't couple there.
if(sqrt(x.at(0)*x.at(0) + x.at(1)*x.at(1) + x.at(2)*x.at(2)) < Ionosphere::innerRadius) {
# TODO drop this warning if it never occurs? To be followed.
cerr << "Triggered mapping back into Earth\n";
Copy link
Owner

Choose a reason for hiding this comment

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

To my surprise, I see this warning being printed quite often in a small testrun I just set up in my fridge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a small test, with coarse resolution, I can see how it would just miss the threshold and step past the 88° limit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can make it more complex and look e.g. at passing an extremum/return point but the more complex we make it, the more I fear we'll have problems once we have non-trivial B configurations.

@ykempf
Copy link
Collaborator Author

ykempf commented Mar 22, 2022

I added one typo fix but more usefully that more detailed reporting to logfile instead of the slurm file. Example output:

Ionosphere: iterations 636 restarts 6 residual 9.283033e-07 potential min -227.628 max 1262.64 difference 1490.27

I thought I wouldn't explicitly add converged/not converged but can be added if requested.

@ykempf
Copy link
Collaborator Author

ykempf commented Mar 30, 2022

So the last two I guess are uncontroversial (the latter was tedious and is useful for debugging, so unless you have a more compact way of writing that is understandable, it would be nice to keep it I think).

Density and temperature are the average of nearest neighbors while velocity comes from the ionospheric potential.
Include tstep and time on line for easier grepping and plotting.
Separate potential min max difference for N and S hemispheres.
@ykempf
Copy link
Collaborator Author

ykempf commented Apr 6, 2022

Comments on bdbbff0? This makes life easier when grepping/plotting from the logfile.

@ykempf
Copy link
Collaborator Author

ykempf commented Apr 6, 2022

Of course I forgot f0450bb that gives the separate N and S potential min and max.

@ykempf
Copy link
Collaborator Author

ykempf commented Apr 6, 2022

21f43b6 fixes the coupling interval feature so that the Vlasov boundary VDFs are also only updated at the set interval like the ionosphere. The if condition in the vlasovBoundary function is a bit ugly due to the counter being incremented in main(), feel free to suggest a better solution if this hurts your eye too much.

@ykempf
Copy link
Collaborator Author

ykempf commented Apr 6, 2022

5ad350f is a bit WIP. Let me know if this is good enough for now, then I'll clean up the commented regions. If you have another idea, please post. Of course we can also ponder whether we want to find which derivatives to zero out and which to keep, but I'm not sure we want to go there...

@ursg
Copy link
Owner

ursg commented Apr 6, 2022

Alrighty, thumbs up to the whole bunch of recent commits there!
I still disagree in some places about [] vs. .at() (especially if the array being indexed is a statically sized std::array that obviously will not have an out-of-bounds access, where the .at() call prevents the compiler from autovectorizing).

But I cherry-picked them over nonetheless. :)

}

// Calculate Hall and Pedersen conductivity coefficient based on charge carrier density
const Real Bval = 5e-5; // TODO: Hardcoded B strength here?
const Real NO_gyroFreq = physicalconstants::CHARGE * Bval / (31*physicalconstants::MASS_PROTON); // Ion (NO+) gyration frequency
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's one diff not due to my .at() craze.

Copy link
Owner

Choose a reason for hiding this comment

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

However, gumics also uses 31 here. I'm still not sure if that is not just simply wrong.

Comment on lines -926 to +930
Real halfdx = 1000 * 0.5 * (atmosphere[h].altitude - atmosphere[h-1].altitude);
Real halfCH = halfdx * 0.5 * (atmosphere[h-1].hallcoeff + atmosphere[h].hallcoeff);
Real halfCP = halfdx * 0.5 * (atmosphere[h-1].pedersencoeff + atmosphere[h].pedersencoeff);
Real halfCpara = halfdx * 0.5 * (atmosphere[h-1].parallelcoeff + atmosphere[h].parallelcoeff);
Real halfdx = 1000 * 0.5 * (atmosphere.at(h).altitude - atmosphere.at(h-1).altitude);

nodes[n].parameters[ionosphereParameters::SIGMAP] += (electronDensity[h]+electronDensity[h-1]) * halfCP;
nodes[n].parameters[ionosphereParameters::SIGMAH] += (electronDensity[h]+electronDensity[h-1]) * halfCH;
nodes[n].parameters[ionosphereParameters::SIGMAPARALLEL] += (electronDensity[h]+electronDensity[h-1]) * halfCpara;
nodes.at(n).parameters.at(ionosphereParameters::SIGMAP) += halfdx * 0.5 * (electronDensity.at(h)*atmosphere.at(h).pedersencoeff + electronDensity.at(h-1)*atmosphere.at(h-1).pedersencoeff);
nodes.at(n).parameters.at(ionosphereParameters::SIGMAH) += halfdx * 0.5 * (electronDensity.at(h)*atmosphere.at(h).hallcoeff + electronDensity.at(h-1)*atmosphere.at(h-1).hallcoeff);
nodes.at(n).parameters.at(ionosphereParameters::SIGMAPARALLEL) += halfdx * 0.5 * (electronDensity.at(h)*atmosphere.at(h).parallelcoeff + electronDensity.at(h-1)*atmosphere.at(h-1).parallelcoeff);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is also still open.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, took that one along.

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