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

Write out full bgb data x make differences derivatives #786

Merged

Conversation

ykempf
Copy link
Contributor

@ykempf ykempf commented Jun 13, 2023

This combines #711 and #727 to make sure they are consistent.

…hreshold to guide in case of stray small values."

This reverts commit 92cba4d.
…to write_out_full_bgb_data_X_make_differences_derivatives
@ykempf ykempf requested review from ursg and markusbattarbee June 13, 2023 08:46
This was referenced Jun 13, 2023
Copy link
Contributor

@ursg ursg left a comment

Choose a reason for hiding this comment

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

Thumbs up for this. I guess this will somewhat collide with PR #787, but those conflicts should be straightforward to fix.

@ykempf
Copy link
Contributor Author

ykempf commented Jun 20, 2023

The ionosphere test is sensitive. Here I assume that we got one or more cells that were (not) coupled in (or ditched), probably/hopefully due to numerics, and that shows up as a O(10¹⁰ m²) diff in upmapped area. Bummer.

@ykempf
Copy link
Contributor Author

ykempf commented Jun 20, 2023

For the record here: upon closer inspection I see that the diffs of order 10¹⁰ are actually relative O(10⁻⁴) and the diffs in the coordinates mapped are up to maybe 15 km, with the dx being 7500 km in Ionosphere_small. So this comes from differences in the tracing and maybe from the many iterations of the ionosphere solver (every step). The actual upmapped area is identical to the eye and to 4 digits in VisIt.

Copy link
Contributor

@markusbattarbee markusbattarbee left a comment

Choose a reason for hiding this comment

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

Urgh, this datareducer.cpp diff is hideous.

And, what you've now got is a huge amount of duplication, as it has basically a giant construction at the beginning for this one special case of BGB writing.

Wouldn't it be possible that if one is requesting the bgb writeout, the initialization would replace the list of datareducers read from the config with one provided (which has only the bgb values, derivatives, etc)?

The reason I'm also thinking about this is that merging this with Jaro's much more elegant and re-using labmda approach will be a nightmare, whereas if the bgb writing only tampers with the list of reducers to make, it'll be much more straightforward.

Copy link
Contributor

@markusbattarbee markusbattarbee left a comment

Choose a reason for hiding this comment

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

Oh that is so much easier to diff-analyse! Thank you! Still a lot of changes to go through though, I'll try to check them all tomorrow.

Want to check about volumetric d's though: here they aren't being divided by the dx, dy or dz. But looking at derivatives.cpp they also look they are differences, not derivatives?

@markusbattarbee
Copy link
Contributor

Oh, one more thing, "g_b_background_vol and fg_derivs_b_background should be added to the allowed output parameters list, since they are new.

@ykempf
Copy link
Contributor Author

ykempf commented Jun 29, 2023

Summary of our discussion with Markus, so I remember what to code after lunch:

  • Adding some more bells, whistles and hoops to get fg_dperbidi out is inconvenient now, and realistically won't be of much use;
  • Thence we can ditch computing and storing fg_dbgbidi components;
  • However we'll keep the fg_dbgbivoldi computation, storage and one-off IO as well as their vg averages;
  • And I'll repair the git wreck that caused me to actually revert what was computing the fg_dbgbidi to not do it, when we want it, in backgroundfield.cpp.

@alhom
Copy link
Contributor

alhom commented Jun 29, 2023

Oh, since there will be a big bunch of outputs now, should we actually use instead of separate fg_dperbxdz some construct like fg_dperbidj/fg_dperbxdz to group the outputs? Or even store them as 9-component vectors (... which VisIt handles as tensors directly)?

@ykempf ykempf mentioned this pull request Jul 3, 2023
@ykempf
Copy link
Contributor Author

ykempf commented Sep 21, 2023

There must be rotten reference data, this PR doesn't touch the insides of the code, only IO.

@ykempf
Copy link
Contributor Author

ykempf commented Sep 25, 2023

Right so this has two bugs in the merge to get all variables written out in datareducer.cpp. I have a fix ready to push. These bugs cause what we see in this PR's CI tests under Ionosphere_small, namely that variables were not found except for fg_b as that DROs continue was not put in an if for the option writing all variables.
This however exposes an issue in the reporting (here of Ionosphere_small: only fg_b 0, 1 and 2 had output for the diff, yet the report is shouting about fg_e having diffs, though it prints the empty value there as no value was detected. I think a test should be reported as properly failed when vlsvdiff doesn't find a variable.
So I have a fix ready to push, I can hold it up until the CI testing is fixed. As discussed on Zulip, the diffs reported don't match those obtained by running the testpackage manually.

@ykempf
Copy link
Contributor Author

ykempf commented Sep 25, 2023

Hooray

@ykempf
Copy link
Contributor Author

ykempf commented Sep 25, 2023

So, merge this?

@ykempf ykempf merged commit 5490a43 into fmihpc:dev Sep 25, 2023
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.

4 participants